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

Add support for Referrer-Policy header #3775

Merged
merged 1 commit into from
Apr 28, 2018
Merged

Conversation

geek
Copy link
Member

@geek geek commented Apr 6, 2018

No description provided.

test/headers.js Outdated
@@ -438,6 +438,41 @@ describe('Headers', () => {
expect(res.headers['x-download-options']).to.equal('noopen');
expect(res.headers['x-content-type-options']).to.equal('nosniff');
});

it('does not return the referrer-policy header whe security.referrer is false', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

It might be good to also ensure that the default behaves like false, since it would not likely be picked-up by another test.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@devinivy added another test for the default and fixed that whe typo.

API.md Outdated
- `referrer` - controls the ['Referrer-Policy'](https://www.w3.org/TR/referrer-policy/) header value:
- `false` - the 'Referrer-Policy' header will not be sent with responses. This is the default value.
- `''` - empty string indicating that the Referrer-Policy will be defined elsewhere.
- `'no-referrer'` - never include the referrer header.
Copy link
Contributor

Choose a reason for hiding this comment

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

@geek this is question more than feedback on PR - what is the difference here between false and 'no-referrer'?

Copy link
Member Author

Choose a reason for hiding this comment

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

false doesn’t send the Referrer-Policy. no-referrer will send the RP header

Copy link
Contributor

Choose a reason for hiding this comment

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

The description is misleading...

@johnbrett
Copy link
Contributor

johnbrett commented Apr 7, 2018 via email

API.md Outdated
- `referrer` - controls the ['Referrer-Policy'](https://www.w3.org/TR/referrer-policy/) header value:
- `false` - the 'Referrer-Policy' header will not be sent with responses. This is the default value.
- `''` - empty string indicating that the Referrer-Policy will be defined elsewhere.
- `'no-referrer'` - never include the referrer header.
Copy link
Contributor

Choose a reason for hiding this comment

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

The description is misleading...

API.md Outdated
@@ -3394,6 +3394,18 @@ following options:
- `noSniff` - boolean controlling the 'X-Content-Type-Options' header. Defaults to `true` setting
the header to its only and default option, `'nosniff'`.

- `referrer` - controls the ['Referrer-Policy'](https://www.w3.org/TR/referrer-policy/) header value:
- `false` - the 'Referrer-Policy' header will not be sent with responses. This is the default value.
- `''` - empty string indicating that the Referrer-Policy will be defined elsewhere.
Copy link
Contributor

Choose a reason for hiding this comment

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

"elsewhere"?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, can this be "empty" instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, we can change this, but it won't be consistent with the referrer policy options, which is to send a literal empty string. Up to you, do you want me to change this?

Copy link
Contributor

Choose a reason for hiding this comment

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

It is just so odd to have an empty string as value. We can start with it as is and see if people are confused by it.

Copy link
Member Author

Choose a reason for hiding this comment

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

"elsewhere" is the language used by w3, which really means in html meta-headers. I'll make this clearer

API.md Outdated
- `'origin-when-cross-origin'` - the referrer includes the full path for same-origin requests but only the origin components of the URL are included for cross origin requests.
- `'strict-origin-when-cross-origin'` - same as `'origin-when-cross-origin'` but the referrer will be omitted when going from HTTPS to HTTP.
- `'unsafe-url'` - the referrer will always be included with the full URL.

Copy link
Contributor

Choose a reason for hiding this comment

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

Basically, the descriptions are confusing because they are about the HTTP policy, not the hapi server actions. Needs to rewrite them to read "Informs the client to...".

Copy link
Member Author

Choose a reason for hiding this comment

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

makes sense, I'll update

@hueniverse hueniverse self-assigned this Apr 23, 2018
@hueniverse hueniverse added feature New functionality or improvement security Issue with security impact labels Apr 23, 2018
@geek
Copy link
Member Author

geek commented Apr 24, 2018

@hueniverse updated

@hueniverse hueniverse added this to the 17.3.2 milestone Apr 28, 2018
@hueniverse hueniverse merged commit eb2c3a6 into hapijs:master Apr 28, 2018
@lock
Copy link

lock bot commented Jan 9, 2020

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature New functionality or improvement security Issue with security impact
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants