Skip to content

Harden SocialSample FailureMessage rendering#67995

Draft
cincuranet with Copilot wants to merge 7 commits into
mainfrom
copilot/harden-socialsample-handling-failuremessage
Draft

Harden SocialSample FailureMessage rendering#67995
cincuranet with Copilot wants to merge 7 commits into
mainfrom
copilot/harden-socialsample-handling-failuremessage

Conversation

Copilot AI commented Jul 24, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Harden SocialSample FailureMessage rendering

  • You've read the Contributor Guide and Code of Conduct.
  • You've included unit or integration tests for your change, where applicable.
  • You've included inline docs for your change, where applicable.
  • There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.

HTML-encode SocialSample FailureMessage before rendering

Description

SocialSample wrote Request.Query["FailureMessage"] directly into an HTML response. This change encodes that value before rendering so query content cannot break the page or inject markup.

  • Endpoint hardening

    • Encodes the /error endpoint's FailureMessage query value with HtmlEncoder.Default.Encode(...) before writing it into the response body.

  • Coverage

    • Adds a focused test that hosts SocialSample, calls /error with markup-bearing input, and verifies the encoded value is rendered instead of the raw string.
await response.WriteAsync(
    "An remote failure has occurred: " +
    HtmlEncoder.Default.Encode(context.Request.Query["FailureMessage"]) +
    "<br>");

Copilot AI linked an issue Jul 24, 2026 that may be closed by this pull request
Harden SocialSample's handling of FailureMessage #67994
Open
@cincuranet
Harden SocialSample FailureMessage rendering
20926a2 
Co-authored-by: cincuranet <4540597+cincuranet@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix escaping of FailureMessage in SocialSample Harden SocialSample FailureMessage rendering Jul 24, 2026
Copilot AI requested a review from cincuranet July 24, 2026 12:01
javiercn
javiercn approved these changes Jul 24, 2026
View reviewed changes

@javiercn javiercn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We likely don't need the test for the sample

cincuranet
cincuranet requested changes Jul 24, 2026
View reviewed changes
Comment thread src/Security/Authentication/test/Microsoft.AspNetCore.Authentication.Test.csproj
Comment thread src/Security/Authentication/test/SocialSampleTests.cs
Comment thread src/Security/Authentication/samples/SocialSample/Startup.cs
@cincuranet
Address SocialSample review feedback
ae21c0b 
Co-authored-by: cincuranet <4540597+cincuranet@users.noreply.github.com>
Copilot AI requested a review from cincuranet July 24, 2026 12:30
@cincuranet

cincuranet commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

@copilot Look around in other samples for similar missing escaping. If you find some, fix it as well.

javiercn
javiercn reviewed Jul 24, 2026
View reviewed changes

@javiercn javiercn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I retract my comment. I didn't realize that we already have a test project for these. If that is the case, it is ok to have the test. @copilot

@cincuranet
Changes before error encountered
ab8d0a8 
Agent-Logs-Url: https://github.com/dotnet/aspnetcore/sessions/efe9cfb2-0353-4123-9bf1-aa24ed3e6081

Co-authored-by: cincuranet <4540597+cincuranet@users.noreply.github.com>
@javiercn
Harden sample HTML attribute rendering
a2cb944 
Co-authored-by: javiercn <6995051+javiercn@users.noreply.github.com>

Copilot AI commented Jul 24, 2026

Copy link
Copy Markdown
Contributor Author

@copilot Look around in other samples for similar missing escaping. If you find some, fix it as well.

Addressed in a2cb944. I checked the other sample HTML responses for similar request-derived rendering and hardened two additional spots: CookiePolicySample now encodes request path/path-base values before using them in links, and HotAddSample now encodes PathBase before writing it into link/form attributes.

Copilot AI requested a review from javiercn July 24, 2026 12:45
cincuranet
cincuranet requested changes Jul 24, 2026
View reviewed changes

@cincuranet cincuranet left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@copilot And add test back per #67995 (review).

Comment thread src/Security/Authentication/samples/SocialSample/Startup.cs Outdated
response.ContentType = "text/html";
await response.WriteAsync("<html><body>");
await response.WriteAsync("An remote failure has occurred: " + context.Request.Query["FailureMessage"] + "<br>");
await response.WriteAsync("An remote failure has occurred: " + HtmlEncoder.Default.Encode(context.Request.Query["FailureMessage"]) + "<br>");

@cincuranet cincuranet Jul 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@copilot This can use the HtmlEncode that is now available.

Copilot AI Jul 24, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in b3e62c5.

@cincuranet
Restore SocialSample test coverage
b3e62c5 
Co-authored-by: cincuranet <4540597+cincuranet@users.noreply.github.com>

Copilot AI commented Jul 24, 2026

Copy link
Copy Markdown
Contributor Author

@copilot And add test back per #67995 (review).

Addressed in b3e62c5.

Copilot AI requested a review from cincuranet July 24, 2026 13:12
cincuranet
cincuranet requested changes Jul 24, 2026
View reviewed changes
Comment thread src/Security/Authentication/test/SocialSampleTests.cs Outdated
using var host = await CreateHost();
using var client = host.GetTestClient();

var failureMessage = "<script>alert('xss') & \"boom\"</script>";

@cincuranet cincuranet Jul 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@copilot Change this to just <h1>test</h1>.

Copilot AI Jul 24, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 5610291.

@cincuranet
Simplify SocialSample test payload
5610291 
Co-authored-by: cincuranet <4540597+cincuranet@users.noreply.github.com>
Copilot AI requested a review from cincuranet July 24, 2026 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@javiercn javiercn Awaiting requested review from javiercn
@cincuranet cincuranet Awaiting requested review from cincuranet
@halter73 halter73 Awaiting requested review from halter73 halter73 will be requested when the pull request is marked ready for review halter73 is a code owner
@BrennanConroy BrennanConroy Awaiting requested review from BrennanConroy BrennanConroy will be requested when the pull request is marked ready for review BrennanConroy is a code owner
Copilot code review Copilot Awaiting requested review from Copilot Copilot will automatically review once the pull request is marked ready for review

Requested changes must be addressed to merge this pull request.

Assignees

@cincuranet cincuranet

Copilot code review Copilot

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Harden SocialSample's handling of FailureMessage

3 participants

@cincuranet @javiercn