Conversation
maartenba
left a comment
There was a problem hiding this comment.
Looks solid!
Note to self: needs duplication into v8 samples folder for now.
| CancellationToken ct) | ||
| { | ||
| // Merge default scopes into whatever the document already declares | ||
| var existing = context.Document.Scope?.Split(' ', StringSplitOptions.RemoveEmptyEntries) ?? []; |
There was a problem hiding this comment.
It feels a little odd that a method named validate is modifying the contents of what it is being validating
There was a problem hiding this comment.
I am going for a similar thing to what the rest of the IdentityServer validators do - we're building a valid model based on the input that we got plus custom logic. Does that make sense to you, or do you think we should rename?
| ClientName = document.ClientName, | ||
| LogoUri = document.LogoUri?.ToString(), | ||
| ClientUri = document.ClientUri?.ToString(), | ||
| RedirectUris = document.RedirectUris?.Select(u => u.ToString()).ToList() ?? [], |
There was a problem hiding this comment.
I don't see that we're validating the RedirectUris from the document anywhere. Are there rules in the spec which require anything specific about them?
There was a problem hiding this comment.
Should (may) be same-origin as the client_id per spec:
An authorization server may have restrictions on what it accepts as valid redirect_uris, for instance, limiting them to the same-origin as the client_id or client_uri properties.
|
Maybe also extend DiscoveryResponseGenerator to add |
I'm setting this via configuration. If CIMD was built into identity server I'd definitely want to make that automatic in the generator. But as a sample I think this is ok. Do you have something in mind to favor the generator? |
No, I was hoping it makes it all the way to identity server as a feature, alongside DCR :) |
| @@ -0,0 +1 @@ | |||
| window.location.href = document.querySelector("meta[http-equiv=refresh]").getAttribute("data-url"); | |||
Check failure
Code scanning / CodeQL
DOM text reinterpreted as HTML High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 10 hours ago
In general, the fix is to avoid using arbitrary DOM text directly as a navigation target and instead validate or normalize the URL before assigning it to window.location.href. That means: parse the URL, restrict allowed protocols (typically only http:/https: or only same-origin relative URLs), and fall back to a safe default if the value is missing or invalid.
For this specific file IdentityServer/v7/CIMD/CIMD.IdentityServer/wwwroot/js/signin-redirect.js, we can keep the existing behavior (redirect to the URL indicated in the meta[http-equiv=refresh] tag) but interpose a small validation step. A practical approach that does not require new libraries is:
- Read the
data-urlvalue into a variable. - If it is falsy, do nothing (or optionally redirect to
/). - If it starts with
/(a relative path) or a known safe prefix (e.g., a known base path), allow it as-is. - Otherwise, try to construct a
URLobject withdata-urlandwindow.location.originas base, and only allow navigation if:- the resulting URL’s protocol is
http:orhttps:, and - the origin matches
window.location.origin(to enforce same-origin), or at least protocol/host are acceptable.
- the resulting URL’s protocol is
- If validation fails, do not redirect (or redirect to a safe fallback like
/).
Because we are only allowed to edit the shown snippet, the change will be local to line 1: introduce local variables around the existing expression, and include a small validation routine inline without extra imports. This preserves functionality when data-url is a normal relative URL, while blocking malicious schemes like javascript: or cross-origin redirects if those are not desired.
| @@ -1 +1,30 @@ | ||
| window.location.href = document.querySelector("meta[http-equiv=refresh]").getAttribute("data-url"); | ||
| (function () { | ||
| var meta = document.querySelector("meta[http-equiv=refresh]"); | ||
| if (!meta) { | ||
| return; | ||
| } | ||
|
|
||
| var rawUrl = meta.getAttribute("data-url"); | ||
| if (!rawUrl) { | ||
| return; | ||
| } | ||
|
|
||
| // Allow simple relative paths (starting with "/") without further parsing. | ||
| if (rawUrl.charAt(0) === "/") { | ||
| window.location.href = rawUrl; | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| // Resolve against current origin to prevent javascript: and other unsafe schemes. | ||
| var resolved = new URL(rawUrl, window.location.origin); | ||
|
|
||
| // Only allow http/https and same-origin redirects. | ||
| if ((resolved.protocol === "http:" || resolved.protocol === "https:") && | ||
| resolved.origin === window.location.origin) { | ||
| window.location.href = resolved.href; | ||
| } | ||
| } catch (e) { | ||
| // Invalid URL: do not redirect. | ||
| } | ||
| }()); |
This adds a sample showing how to implement CIMD in IdentityServer to authenticate MCP clients like VS Code.
Apologies for the large number of files, but most of it is standard IdentityServer and a copy of the weather MCP server.
The interesting dozen or so CIMD-specific files are all in CIMD.IdentityServer/. Everything else is copied from McpDemo with namespace changes.