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

Onion location paths #733

Closed
wants to merge 2 commits into from
Closed

Onion location paths #733

wants to merge 2 commits into from

Conversation

chigby
Copy link
Contributor

@chigby chigby commented Sep 28, 2020

This pull request fixes the problem with the Onion-Location header always referring to the home page, rather than the page path requested by each individual page.

Fixes #724

@chigby chigby force-pushed the onion-location-paths branch 2 times, most recently from 6c8c4c2 to 2fd4957 Compare September 28, 2020 19:42
Copy link
Contributor

@SaptakS SaptakS left a comment

Choose a reason for hiding this comment

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

So right now if the footer_settings.securedrop_onion_address is set to an onion address without https:// in the beginning (which is the case in createdevdata), the urljoin() function technically just replaces the onion address with just the request.path. Hence only the path is returned in that case. I am not sure in which format onion address is saved in prod. Maybe can add a check of some sort to append https:// in the beginning?

@chigby
Copy link
Contributor Author

chigby commented Oct 5, 2020

I think there is a validator on the model itself:

securedrop_onion_address = models.CharField(
'SecureDrop Onion Address',
max_length=255,
default='secrdrop5wyphb5x.onion',
validators=[
RegexValidator(regex=r'\.onion$', message="Enter a valid .onion address."),
URLValidator(schemes=['http', 'https'], message='Onion address must be a valid URL beginning with http or https'),
],
help_text='Address for the securedrop.org onion service. Displayed in the site footer and the Onion-Location header. Must begin with http or https and end with .onion',
)

Though I also do see the benefit of ensuring we don't do the wrong thing in this middleware either, regardless of the overall format.

@chigby
Copy link
Contributor Author

chigby commented Oct 5, 2020

Maybe create dev data is what needs to be changed. Hm.

@SaptakS
Copy link
Contributor

SaptakS commented Oct 5, 2020

Maybe create dev data is what needs to be changed. Hm.

Possible. Hence marked it as a comment and not request changes. I think given the validator, probably safe to just update create dev data

@chigby
Copy link
Contributor Author

chigby commented Oct 5, 2020

Added! Thanks for the review.

@chigby chigby force-pushed the onion-location-paths branch from 66785d5 to 5cbaafc Compare October 13, 2020 13:28
The Onion Location Middleware requires this setting to act as a "base"
URL in order to form the correct header value, so we need the
`http://` in this setting.  The validation already tries to make sure
this happens, so we ought to have it in the default value as well.
@harrislapiroff
Copy link
Contributor

Given that @maeve-fpf is deploying the k8s powered version of this site tomorrow which will include nginx managed onion-location headers, this PR is no longer necessary (I didn't get my review together in time, whoops). Closing without merging.

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.

Onion location header redirects to homepage
3 participants