Normalize data protection app discriminator #41095
Conversation
ghost
added
the
|
Thoughts @blowdart we will just trim a single either direction slash by default |
|
Is that not going to end up with a null discriminator in some circumstances? That feels bad. |
|
You mean if a single "/" is the content root? Trim would return empty string right? |
|
Yup. That feels odd to me. |
|
But does that matter? This is just to disambugiate apps, in practice people wouldn't have an app that's "/" and an app thats "" right? As long as we don't generate collisions between real distinct apps, this is an implementation detail, especially since this is just when the content root path is used as opposed to using an explicitly set application name |
|
I guess, it's just an empty discriminator feels wrong somehow. |
HaoK
requested review from
Tratcher,
adityamandaleeka,
sebastienros and
wtgodbe
|
|
||
| public string? Discriminator => _hosting?.ContentRootPath; | ||
| // Note: ContentRootPath behavior depends on the version, sometimes it has a trailing slash, we normalize by default by removing a trailing slash | ||
| public string? Discriminator => _hosting?.ContentRootPath?.TrimEnd(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar); |
There was a problem hiding this comment.
Should it be stored in a field at construction instead of computed of every invocation?
There was a problem hiding this comment.
We don't know how IHostEnvironment implements ContentRootPath right? Seems like the caching would belong there
Co-authored-by: Aditya Mandaleeka <[email protected]>
|
Assuming this is backported to 6, does this mean we'll now have consistent behavior across 5, 6, and 7 regardless of the entry point APIs? |
|
That's the point, but I'm not even sure we need this to be backported to 6 for compat, since I think its only in 7 that we have the mismatch between trailing slash and no slash. |
|
There was a similar break in 6 so it's not compatible with 7, even after this change in 7. |
|
Which direction do you mean? In 7 it will handle either situation, since it will trim, we don't care about 7.0 cookies going to 6 as much do we? |
|
We want cookies to be forwards and backwards compatible where possible. Cookies from 6 are currently neither. I think this fix makes 5 and 7 compatible, but we'd still have to fix 6. |
I don't think this is generally true, I believe the only apps that were affected were the new minimal style that hit the difference in content root, so it would be very hard for them to in practice have compat issues going backwards to 5. Going forward to 7 won't have an issue after this fix. But whatever, this is a cheap fix to backport to 6.0 anyways so I guess it doesn't matter |
|
/backport to release/6.0 |
|
Started backporting to release/6.0: https://github.com/dotnet/aspnetcore/actions/runs/2204307835 |
|
@HaoK backporting to release/6.0 failed, the patch most likely resulted in conflicts: $ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: Normalize data protection app discriminator
Using index info to reconstruct a base tree...
M src/DataProtection/DataProtection/src/Internal/HostingApplicationDiscriminator.cs
M src/DataProtection/DataProtection/test/HostingTests.cs
Falling back to patching base and 3-way merge...
Auto-merging src/DataProtection/DataProtection/test/HostingTests.cs
CONFLICT (content): Merge conflict in src/DataProtection/DataProtection/test/HostingTests.cs
Auto-merging src/DataProtection/DataProtection/src/Internal/HostingApplicationDiscriminator.cs
CONFLICT (content): Merge conflict in src/DataProtection/DataProtection/src/Internal/HostingApplicationDiscriminator.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Normalize data protection app discriminator
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128Please backport manually! |
For #40964