Skip to content
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

Extend redirect URI validation with protocol check #850

Merged
merged 1 commit into from
Feb 21, 2020
Merged

Extend redirect URI validation with protocol check #850

merged 1 commit into from
Feb 21, 2020

Conversation

Rulox
Copy link
Contributor

@Rulox Rulox commented Feb 17, 2020

Proposed changes

  • Update docs with proper variables
  • Make sure that validation of Redirect URLs (both in action and error pages) check for the protocol of the URL to be a fully working URL (this is to avoid NGINX to try to open local files). Now a redirect URL must contain ://.

Copy link
Contributor

@Dean-Coakley Dean-Coakley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm pending the above change

Copy link
Contributor

@pleshakov pleshakov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Rulox thanks. a few notes about using $upstream_status instead of $status

@Rulox Rulox requested a review from pleshakov February 19, 2020 11:55
Copy link
Contributor

@pleshakov pleshakov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Rulox Thanks! The changes looks good!

Do we still support upstream_status in the error page redirect tho?

Not sure if this variable can be useful. The problem is that it could contain multiple values:

$upstream_status
keeps status code of the response obtained from the upstream server. Status codes of several responses are separated by commas and colons like addresses in the $upstream_addr variable. If a server cannot be selected, the variable keeps the 502 (Bad Gateway) status code.

Considering that, I don't think it could be useful in redirects.

Copy link
Contributor

@tellet tellet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@Rulox Rulox merged commit ae85531 into master Feb 21, 2020
@Rulox Rulox deleted the fixes branch February 21, 2020 12:24
@pleshakov pleshakov added the enhancement Pull requests for new features/feature enhancements label Mar 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Pull requests for new features/feature enhancements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants