Conversation
| } | ||
|
|
||
| /** Swaps a refresh token for a new one, so a leaked refresh token stops working after its next use. */ | ||
| async rotateRefreshToken( |
There was a problem hiding this comment.
if attacker steals refresh and will use it before legit user, legit user will be kicked and attacker will be left in session. OAuth 2.1 strongly recommends to revoke full grant on double refresh reuse
| path: OAUTH_PATHS.authorize, | ||
| noAuth: true, | ||
| handler: async (input) => oauthResponse(input, async () => { | ||
| const consentPageUrl = await oauth.authorize(input.query); |
There was a problem hiding this comment.
this method exposes reflector proxy possibility because does outgoing cache without any cache/noAuth
Ideas?
| // The JSON round trip drops functions and undefined values that handler responses may carry, which YAML cannot serialize. | ||
| function serializeToolOutput(output: unknown): string { | ||
| return typeof output === 'string' ? output : JSON.stringify(output, null, 2); | ||
| return typeof output === 'string' ? output : YAML.stringify(JSON.parse(JSON.stringify(output))); |
There was a problem hiding this comment.
@srelon what will be here if handler returns undefined? totally possible =)
|
|
||
| const BEARER_SECRET_RE = /^Bearer (afmcp_[A-Za-z0-9_-]+)$/i; | ||
| const BEARER_TOKEN_RE = /^Bearer (\S+)$/i; | ||
| const MCP_PATH = '/mcp'; |
There was a problem hiding this comment.
@srelon this is SSOT violation (partially was already here but now with OAuth can lead to bad consequences) better return mcpUrl somewhere e.g. GET /mcp/auth-secrets
There was a problem hiding this comment.
@ivictbor you can write in more detail? I'm not sure that I understood correctly
| redirectUri: params.redirect_uri, | ||
| codeChallenge: params.code_challenge, | ||
| state: typeof query.state === 'string' ? query.state : undefined, | ||
| loopbackRedirect: isLoopbackRedirectUri(params.redirect_uri), |
There was a problem hiding this comment.
@srelon I think u parse redfirect url 4 times, but it can be done once - minor but anyway, as concept of wasting CPU loops - in terms of code perfection better do it once and reuse
| * Returns a copy of `target` without the value at `pathParts`. Only objects along the path are copied, | ||
| * because handler responses may share nested objects with the AdminForth config. | ||
| */ | ||
| function omitPath(target: unknown, pathParts: string[]): unknown { |
There was a problem hiding this comment.
nod bad but I would say it is YAGNI violation and more like upfront optimisation
omitPath/omitPaths plus GET_RESOURCE_FRONTEND_ONLY_PATHS add a small path DSL (dot-separated keys, a [] suffix that maps over arrays, recursive copy-on-write). It is used in exactly one place, for 4 hardcoded paths, and the response shape is fully known from AdminForthResourceFrontend. Nothing else needs path-based omission, so the abstraction is speculative.
It makes it a little bit hardread - reader has to understand the DSL to find out what actually gets removed, and the unknown in/out drops the typing we already have.
Plain destructuring does the same thing, is typed, and stays copy-on-write, so the response still never mutates the shared config objects.
kinda
// Drops frontend-only parts of the get_resource response that are useless for MCP clients and only waste their context.
// Copies instead of deleting: the response shares nested objects with the AdminForth config.
function withoutFrontendOnlyFields({ resource }: GetResourceOutput): GetResourceOutput {
const { pageInjections, ...options } = resource.options;
return {
resource: {
...resource,
columns: resource.columns.map(({ filterOptions, components, ...column }) => column),
options: {
...options,
actions: options.actions?.map(({ customComponent, ...action }) => action),
},
},
};
}
project: (output, { detailed }) => (
detailed
? withoutFrontendOnlyFields(output as GetResourceOutput)
: essentialResource(output as GetResourceOutput)
),
well readable
2 participants
No description provided.