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

Prevent truncation for large origin-relative domains #427

Closed
wants to merge 4 commits into from

Conversation

yadij
Copy link
Contributor

@yadij yadij commented Jun 29, 2019

The domain in a URL must obey hostname length restrictions after
append_domain is applied, not just MAX_URL for the normalized
absolute-URL.

@yadij yadij added the S-waiting-for-more-reviewers needs a reviewer and/or a second opinion label Jun 29, 2019
Copy link
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.

  1. Please avoid code duplication.
  2. Please do not give the end user the ability to flood cache.log with useless level-1 messages. If you insist on logging something at level 1, then add a logging limit (e.g., increase the level after the first 10 level-1 messages).
  3. Please do not change !strchr() "not there" mnemonic to a lot less intuitive strchr() == 0. The latter comparison with zero is good for strcmp() but not for strchr().

@yadij yadij removed the S-waiting-for-more-reviewers needs a reviewer and/or a second opinion label Jul 4, 2019
@yadij yadij added the S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box label Jul 4, 2019
Copy link
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.

I only have one semi-important correction. The rest is polishing.

src/anyp/Uri.cc Outdated Show resolved Hide resolved
src/anyp/Uri.cc Outdated Show resolved Hide resolved
src/anyp/Uri.h Show resolved Hide resolved
src/anyp/Uri.cc Show resolved Hide resolved
@rousskov rousskov added S-waiting-for-author author action is expected (and usually required) and removed S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box labels Jul 8, 2019
... and refactor slightly to prevent underflow.
@yadij yadij added S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box and removed S-waiting-for-author author action is expected (and usually required) labels Jul 11, 2019
@rousskov rousskov added S-waiting-for-author author action is expected (and usually required) and removed S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box labels Jul 11, 2019
@yadij yadij added S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box and removed S-waiting-for-author author action is expected (and usually required) labels Jul 12, 2019
@rousskov rousskov added S-waiting-for-committer privileged action is expected (and usually required) and removed S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box labels Jul 12, 2019
Copy link
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 change requests.

N.B. Please avoid resolving the change requests that you have not requested, regardless of whether you think they are resolved.

@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 Jul 12, 2019
squid-anubis pushed a commit that referenced this pull request Jul 12, 2019
The domain in a URL must obey hostname length restrictions after
append_domain is applied, not just MAX_URL for the normalized
absolute-URL.
@squid-anubis squid-anubis added the M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels label Jul 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 Jul 12, 2019
squidadm pushed a commit to squidadm/squid that referenced this pull request Jul 31, 2019
The domain in a URL must obey hostname length restrictions after
append_domain is applied, not just MAX_URL for the normalized
absolute-URL.
yadij added a commit that referenced this pull request Aug 3, 2019
The domain in a URL must obey hostname length restrictions after
append_domain is applied, not just MAX_URL for the normalized
absolute-URL.
squidadm pushed a commit to squidadm/squid that referenced this pull request Sep 7, 2019
The domain in a URL must obey hostname length restrictions after
append_domain is applied, not just MAX_URL for the normalized
absolute-URL.
squidadm pushed a commit to squidadm/squid that referenced this pull request Sep 8, 2019
The domain in a URL must obey hostname length restrictions after
append_domain is applied, not just MAX_URL for the normalized
absolute-URL.
yadij added a commit that referenced this pull request Sep 8, 2019
The domain in a URL must obey hostname length restrictions after
append_domain is applied, not just MAX_URL for the normalized
absolute-URL.
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
3 participants