Skip to content

UrlHelperBase.IsLocalUrl treats control characters correctly #18134

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 8 commits into from
Closed

UrlHelperBase.IsLocalUrl treats control characters correctly #18134

wants to merge 8 commits into from

Conversation

@gfoidl
Copy link
Member

@gfoidl gfoidl commented Jan 5, 2020

Addresses #18109

@gfoidl
Copy link
Member Author

gfoidl commented Jan 5, 2020

/cc: @pranavkm

gfoidl
gfoidl commented Jan 5, 2020
View reviewed changes
src/Mvc/Mvc.Core/test/Routing/UrlHelperTestBase.cs Outdated

[Theory]
[InlineData("/\n")]
[InlineData("/\n/not-local-url")]
Copy link
Member Author

@gfoidl gfoidl Jan 5, 2020

Choose a reason for hiding this comment

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

Are control characters at other places allowed? I.e. at the end, in the middle.
If so, we need to check the whole url for control characters...

Copy link
Contributor

@pranavkm pranavkm Jan 6, 2020

Choose a reason for hiding this comment

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

AFAIK, HttpRespose.Headers will complain if it sees control characters anywhere a header value. We could choose to emulate it in this code.

@Tratcher \ @blowdart what do you think?

Copy link
Member

@Tratcher Tratcher Jan 6, 2020

Choose a reason for hiding this comment

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

Return false or throw if it has any controls? Returning false would be the most compatible.

Copy link
Contributor

@pranavkm pranavkm Jan 6, 2020

Choose a reason for hiding this comment

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

Return false. But basically apply the same checks as whatever HttpResponse.Headers applies to determine the validity.

Copy link
Member

@Tratcher Tratcher Jan 6, 2020

Choose a reason for hiding this comment

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

Anything less than 0x20, or 0x7F (Delete)

Copy link
Member Author

@gfoidl gfoidl Jan 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Is checking the ascii-range [0x00, 0x7F] enough? As there are more control characters above 0x7F.

@Pilchie Pilchie added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Jan 6, 2020
@pranavkm pranavkm added this to the 5.0.0-preview1 milestone Jan 6, 2020
@pranavkm pranavkm self-assigned this Jan 6, 2020
gfoidl
gfoidl commented Jan 7, 2020
View reviewed changes
gfoidl
gfoidl commented Jan 7, 2020
View reviewed changes
pranavkm
pranavkm suggested changes Jan 21, 2020
View reviewed changes
@GrabYourPitchforks GrabYourPitchforks mentioned this pull request Jan 22, 2020
Merged
@mkArtakMSFT mkArtakMSFT removed this from the 5.0.0-preview4 milestone May 18, 2020
@mkArtakMSFT
Copy link
Contributor

mkArtakMSFT commented Jun 26, 2020

@pranavkm what is pending here? have all the concerns been addressed?

@mkArtakMSFT mkArtakMSFT added the community-contribution Indicates that the PR has been added by a community member label Jul 20, 2020
@pranavkm
Copy link
Contributor

pranavkm commented Sep 9, 2020

Thanks for the PR @gfoidl. We resolved this as part of #25378

@pranavkm pranavkm closed this Sep 9, 2020
@gfoidl gfoidl deleted the urlhelperbase-control-char branch September 10, 2020 12:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@GrabYourPitchforks GrabYourPitchforks GrabYourPitchforks left review comments

@Tratcher Tratcher Tratcher left review comments

+1 more reviewer

@pranavkm pranavkm pranavkm requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

@pranavkm pranavkm

Labels

area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@gfoidl @mkArtakMSFT @pranavkm @GrabYourPitchforks @Tratcher @analogrelay @Pilchie