feat(cas): add helper method for authenticated citation sources#599
feat(cas): add helper method for authenticated citation sources#599scottcmg wants to merge 6 commits into
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Review findingsNew inline comments posted this run:
|
Fail-closed origin check, PascalCase telemetry key, typed test param, network/malformed-URL branch tests, and the correct (scope-less) oauth entry per the ECS reference endpoint's org-member policy. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Pushed fixes for the review 👇
Skipped the integration test for now — conv-agent has no integration suite at all yet, so a one-off here felt out of place. Can do it as a separate pass if we want to stand that up. |
| @@ -0,0 +1,52 @@ | |||
| import { describe, it, expect } from 'vitest'; | |||
There was a problem hiding this comment.
The test file path does not mirror the src/ structure. Per the architecture docs: "tests/unit/ mirrors src/ structure".
- Source:
src/services/conversational-agent/helpers/citation.ts - Expected test path:
tests/unit/services/conversational-agent/helpers/citation.test.ts - Actual path:
tests/unit/helpers/conversational-agent/citation.test.ts
The file should be moved to match the source tree.
Review findingsNew inline comments posted this run:
|
Make the service JSDoc identical to the ServiceModel and use backtick notation for the param type per convention. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
✅ No issues found. Checked for bugs and CLAUDE.md compliance. |
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…thod # Conflicts: # src/services/conversational-agent/conversational-agent.ts
Review findingsOne previously-resolved thread was unresolved this run:
|
Review findingsNew inline comment posted this run:
|
|
| /** Configured SDK origin, used to keep citation downloads on the tenant's host. */ | ||
| private readonly baseUrl?: string; |
There was a problem hiding this comment.
instance.config.baseUrl is typed string on BaseConfig, so this can drop the ? (and then we could remove the !base check in line 114)
| throw new ValidationError({ | ||
| message: `Invalid citation downloadUrl` | ||
| }); |
There was a problem hiding this comment.
nit : there's no interpolation, could be plain single quotes around the message. Same for line 116.
Also shouldn't both be ServerError instead?
| text: async () => statusText, | ||
| headers: { get: () => null }, |
There was a problem hiding this comment.
are these two read anywhere?
| it('throws a ValidationError when the source has no downloadUrl', async () => { | ||
| await expect( | ||
| conversationalAgent.downloadCitationSource(mediaSource({ downloadUrl: undefined })), | ||
| ).rejects.toThrow(/downloadUrl/); |
There was a problem hiding this comment.
Every other error test in this block uses .rejects.toBeInstanceOf(ValidationError). This one uses toThrow(/downloadUrl/). Any particular reason?
| * @throws ValidationError if the source has no `downloadUrl` | ||
| * @throws A typed HTTP error for error responses — e.g. `AuthenticationError` | ||
| * (401), `AuthorizationError` (403), `NotFoundError` (404) — or | ||
| * `NetworkError` on a connection failure, matching other SDK calls |
There was a problem hiding this comment.
Does any method document the errors with the @throw tag? Could you verify.
wherever it is used, I don't think it is in the docs surface.
| * Downloads the document behind a media citation as an authenticated `Blob`. | ||
| * | ||
| * Media citation sources expose a `downloadUrl` pointing at an auth-gated | ||
| * UiPath endpoint; opening it directly sends no token and fails with `401`. | ||
| * This performs the request with the SDK's access token and returns the bytes, | ||
| * with the `Blob` type resolved from the source `mimeType` (falling back to the | ||
| * response Content-Type, then the title's file extension). Use `source.title` | ||
| * as the file name. | ||
| * | ||
| * HTML content is intentionally returned as `application/octet-stream` (a | ||
| * download) rather than `text/html`, so previewing the blob inline can't | ||
| * execute citation markup in your app's origin. |
There was a problem hiding this comment.
could this description be made a little concise?
| throw ErrorFactory.createFromHttpStatus(response.status, errorInfo); | ||
| } | ||
|
|
||
| const blob = await response.blob(); |
There was a problem hiding this comment.
should this be inside try-catch as well?
could we extend the existing try/catch to cover the body read too
something like
try {
const response = await fetch(target.href, {
headers: { Authorization: `Bearer ${token}` }
});
if (!response.ok) {
const errorInfo = await errorResponseParser.parse(response);
throw ErrorFactory.createFromHttpStatus(response.status, errorInfo);
}
blob = await response.blob();
} catch (error: any) {
if (error.type && error.type.includes('Error')) throw error;
throw ErrorFactory.createNetworkError(error);
}
3 participants
Details
Customers have reported issues with the generated PDF citations that are returned as part of the conversational agent response where clicking the inline citation or the source in the source list returns a 401 in a new tab. This is because these citations require an authorized fetch from ECS to display.
This PR is to add a helper method that customers can use to add support for these citation links so they do not have to be responsible for independently determining the wiring needed for support. It adds some security hardening against (1) inline HTML execution and (2) hijacked download requests sending user credentials to a non-UiPath-origin source.