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

Consider http_external_uri when setting the cookie path #18472

Merged
merged 4 commits into from Mar 8, 2024

Conversation

thll
Copy link
Contributor

@thll thll commented Mar 4, 2024

Changes which path is chosen to populate the Path attribute in the authentication cookie.

Previously, the path was chosen as follows, highest priority first:

  1. The path from the URL in the X-Graylog-Server-URL request header
  2. The path from the Origin request header
  3. /

With this PR, the value will be chosen as follows:

  1. The path from the URL in the X-Graylog-Server-URL request header
  2. The path from the http_external_uri configuration setting
  3. /

The Origin header does not contain a path. See here:

The Origin header is similar to the Referer header, but does not disclose the path, and may be null.

It was therefore removed as a potential source for the Path attribute.

The http_external_url does however provide a relevant value for restricting the cookie path. It will therefore now be considered.

Furthermore, to prevent the injection of cookie values via the X-Graylog-Server-URL header, all characters that are not valid for the Path attribute are now stripped.

thll added 4 commits March 4, 2024 11:13
According to the spec, the Origin header will not include a path.
The http_external_url might contain a path which should be
considered.
@thll thll marked this pull request as ready for review March 5, 2024 07:59
@kroepke kroepke self-assigned this Mar 5, 2024
Copy link
Member

@dennisoelkers dennisoelkers left a comment

Choose a reason for hiding this comment

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

Code looks good, tests look fine! Leaving extensive testing to @kroepke.

Copy link
Member

@kroepke kroepke left a comment

Choose a reason for hiding this comment

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

ltgm

@kroepke kroepke merged commit a9039b3 into master Mar 8, 2024
5 checks passed
@kroepke kroepke deleted the fix/cookie-path branch March 8, 2024 15:05
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.

None yet

3 participants