Skip to content

Added set commands for base64url, expiry and ip matching #12

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 8 commits into from
Closed

Added set commands for base64url, expiry and ip matching #12

wants to merge 8 commits into from

Conversation

mevdschee
Copy link

I added a command to do base64url encoding and decoding. I also added an expired and ip_matches command that will compare with the current timestamp and ip address. These are building blocks for an alternative secure link module.

@agentzh
Copy link
Member

agentzh commented Mar 7, 2014

@mevdschee Thank you for your contribution!

Several comments though:

  1. I think we should still make this module compile with earlier versions of NGINX like 1.5.9. I suggest adding macro tests against nginx_version to enable the new functionality conditionally.
  2. Please add guard macros to your header filters for consistency.
  3. Please remove redundant blank lines in some places (like the one before local variable declarations and the one at ngx_http_set_misc_module.c:90).

Will you fix these?

@mevdschee
Copy link
Author

I'll try to adjust the code to fix these issues. Thank you.

@mevdschee
Copy link
Author

I made the adjustments you requested in fd8b988. Does it look good to you?

@agentzh
Copy link
Member

agentzh commented Mar 10, 2014

@mevdschee Thank you for the quick fixes! Some more suggestions:

  1. Please put C variable declarations to the beginning of the code block according to the NGINX coding style (and to please some old C compilers).
  2. Please avoid using things like ndk_palloc_re because I'd like to limit our usage here to the set_var NDK submodule only.

@agentzh
Copy link
Member

agentzh commented Mar 10, 2014

@mevdschee Also, will you mind adding some documentation for the new directives to README? Thanks!

@mevdschee
Copy link
Author

Do you want me to update src/ngx_http_set_base64.c and src/ngx_http_set_base64.h as well?

@agentzh
Copy link
Member

agentzh commented Mar 10, 2014

@mevdschee Preferably in a separate branch and separate pull request :) Thanks!

@mevdschee
Copy link
Author

How do I update the documentation? What did you make those readme's with? I see a reference to "wiki2markdown.pl" in a comment. What is that and where can I get it? Is there an online editor for that ".wiki" format? Does it adhere to any standard?

@agentzh
Copy link
Member

agentzh commented Mar 10, 2014

@mevdschee You're recommended to include your changes in the .wiki file in your patch. You can use the wiki.nginx.org site to test your changes.

@mevdschee
Copy link
Author

Okay.. I updated the code in b51046b. I hope it is good now.

@mevdschee
Copy link
Author

Also, I have found "wiki2markdown.pl" in https://github.com/agentzh/nginx-devel-utils

@mevdschee
Copy link
Author

Okay.. Now how do you do update the plain text README? The wiki and markdown are updated in 29b18d2.

@mevdschee
Copy link
Author

I have found wiki2pod.pl and the pod2text Linux tool. The README is updated in 02e12a3.

@agentzh
Copy link
Member

agentzh commented Mar 12, 2014

@mevdschee Thanks for the updates! I'll try to merge your patch soon :)

@mevdschee
Copy link
Author

NB: I have added IPv6 test cases for ip_matches.

@agentzh
Copy link
Member

agentzh commented Mar 13, 2014

@mevdschee Hmm, seems that you've already put 3 unrelated features in this single pull request. Alas.

Will you split these unrelated features into 3 different branches and create 3 different pull requests separately? Mixing things together makes the review and merge process on my side much harder. I prefer small atomic changes in every pull request. Thank you for your cooperation!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants