Skip to content

Fix request URL generation in reverse proxy configurations#519

Closed
guidovranken wants to merge 6 commits intosquid-cache:masterfrom
guidovranken:prepareAcceleratedURL-buffer-overflow-fix
Closed

Fix request URL generation in reverse proxy configurations#519
guidovranken wants to merge 6 commits intosquid-cache:masterfrom
guidovranken:prepareAcceleratedURL-buffer-overflow-fix

Conversation

@guidovranken
Copy link
Copy Markdown
Contributor

@guidovranken guidovranken commented Dec 10, 2019

No description provided.

Signed-off-by: Guido Vranken <guidovranken@gmail.com>
@squid-prbot
Copy link
Copy Markdown
Collaborator

Can one of the admins verify this patch?

Copy link
Copy Markdown
Contributor

@rousskov rousskov left a comment

Choose a reason for hiding this comment

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

Thanks a lot for working on this!

I committed a few polishing touches to your PR branch. One of them (commit 246989f) appears to expose a problem. Please review my commits and adjust as needed.

@yadij yadij added S-waiting-for-more-reviewers needs a reviewer and/or a second opinion S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box and removed S-waiting-for-more-reviewers needs a reviewer and/or a second opinion labels Dec 11, 2019
Copy link
Copy Markdown
Contributor

@rousskov rousskov left a comment

Choose a reason for hiding this comment

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

Thank you for addressing my concern. Glad the code became much simpler.

I polished the comment wording a little and will clear the PR.

@rousskov rousskov added M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels and removed S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box labels Dec 12, 2019
@rousskov rousskov changed the title Fix stack buffer overflow, data leak in prepareAcceleratedURL Fix request URL generation in reverse proxy configurations Dec 12, 2019
@rousskov rousskov added S-waiting-for-committer privileged action is expected (and usually required) and removed M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels labels Dec 12, 2019
@rousskov
Copy link
Copy Markdown
Contributor

I adjusted PR title to be less developer-specific. I adjusted PR description because the Squid Project uses commit Author field (which should be set to @guidovranken by Anubis), instead of Signed-off-by pseudo-headers.

@guidovranken, please feel free to add yourself to CONTRIBUTORS (IIRC that will eventually happen semi-automatically based on the Author field, but I do not know how often that synchronization happens).

Due to these potentially sensitive changes, I removed M-cleared-for-merge label to give others a chance to weigh in before the PR is auto-merged.

@rousskov
Copy link
Copy Markdown
Contributor

OK to test

@yadij yadij added M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels and removed S-waiting-for-committer privileged action is expected (and usually required) labels Dec 12, 2019
@yadij
Copy link
Copy Markdown
Contributor

yadij commented Dec 12, 2019

LGTM

@squid-anubis squid-anubis added the M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels label Dec 12, 2019
@squid-anubis squid-anubis added M-merged https://github.com/measurement-factory/anubis#pull-request-labels and removed M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels labels Dec 12, 2019
squidadm pushed a commit to squidadm/squid that referenced this pull request Jan 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

M-merged https://github.com/measurement-factory/anubis#pull-request-labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants