FAIRCompliance Part1#82
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds multiple benchmark report artifacts, bumps Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Resolver as Generic Resolver
participant Negotiation as Header Negotiation
participant NodesAPI as Nodes API
participant Manifest as Manifest Service
participant IPFS as IPFS Service
participant Redis as Redis Cache
participant RoCrate as RoCrate Builder
participant Response
Client->>Resolver: HTTP request (Accept header, UA, params)
Resolver->>Negotiation: Determine wantsJsonLd/wantsRdf/isRaw
alt JSON‑LD or RDF requested
Resolver->>Manifest: Fetch manifest / history
Manifest-->>Resolver: Manifest metadata
Resolver->>NodesAPI: Fetch AI keywords (getNodesApiUrl)
NodesAPI-->>Resolver: aiKeywords
Resolver->>Redis: Check cached fileSizes
alt cache miss
Resolver->>IPFS: Request file sizes for assets
IPFS-->>Resolver: fileSizes
Resolver->>Redis: Cache fileSizes
else cache hit
Redis-->>Resolver: fileSizes
end
Resolver->>RoCrate: Build RoCrate JSON‑LD (publisher, baseUrl, fileSizes, aiKeywords)
RoCrate-->>Resolver: JSON‑LD payload
alt Crawler/Assessor UA detected
Resolver->>Response: 200 OK HTML landing page embedding JSON‑LD + Signposting headers
else Standard request
Resolver->>Response: 200 OK JSON‑LD/RDF + Signposting headers
end
else No special format requested
Resolver->>Response: Redirect or standard non-JSON‑LD flow
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/api/v2/resolvers/generic.ts (1)
90-113: Content negotiation is too naive: RDF Accept headers currently return JSON-LD with mismatched semantics.The Accept header parsing is case-sensitive and ignores
q=quality values. More critically,wantsRdfViaHeader(lines 99–103) setsisJsonld = true(line 111), but the actual response is alwaysapplication/ld+json(line 135)—never Turtle, RDF/XML, or N3. This violates HTTP content negotiation semantics: if a client requestsAccept: text/turtleand the server responds withContent-Type: application/ld+json, the client expects RDF in Turtle format, not JSON-LD.Recommended action:
Either implement Turtle/RDF/XML serialization, or return406 Not Acceptablefor those formats and clarify that only JSON-LD is supported. If F-UJI requires RDF formats, confirm it accepts JSON-LD as a fallback; otherwise, this configuration does not meet the stated goal.As a starting point, use Express' built-in
req.accepts([...])(theacceptspackage is already a dependency) for proper content negotiation with quality value support:- const acceptHeader = req.headers.accept || ""; + const acceptHeader = (req.headers.accept || "").toLowerCase(); const isApiRequest = acceptHeader.includes("application/json") && !acceptHeader.includes("text/html"); - const wantsJsonLdViaHeader = ... - const wantsRdfViaHeader = ... + const accepted = req.accepts([ + "application/ld+json", + "application/json", + "text/html", + "*/*", + ]); + const wantsJsonLdViaHeader = accepted === "application/ld+json"; + const wantsRdfViaHeader = false; // RDF formats not yet supported
♻️ Duplicate comments (3)
benchmark/benchmark_after_v3.txt (1)
1-233: Consider excluding benchmark artifacts from version control.Same recommendation as the previous benchmark files: benchmark reports should be excluded from version control via
.gitignoreand stored in a dedicated artifact/metrics storage system instead.benchmark/benchmark_before_v2.txt (1)
1-233: Consider excluding benchmark artifacts from version control.Same recommendation applies to this file: exclude via
.gitignoreand store in a dedicated artifact storage system.benchmark/benchmark_before_redis.txt (1)
1-233: Consider excluding benchmark artifacts from version control.Same recommendation applies to this file: exclude via
.gitignoreand store in a dedicated artifact storage system.
🧹 Nitpick comments (4)
benchmark/benchmark_before.txt (1)
1-234: Benchmark artifacts: consider keeping these out of the main repo history (or clearly mark as generated).
This is fine as a snapshot, but it will add churn and repo weight; also double-check it contains no sensitive env info beyond localhost/commit/timestamps.benchmark/benchmark_after.txt (1)
1-234: Benchmark artifacts: same housekeeping note as other benchmark files.
If these are meant for PR comparison only, an alternative is CI-uploaded artifacts or a single rolling file.benchmark/benchmark_after_v2.txt (1)
1-234: Benchmark artifacts: same housekeeping note as other benchmark files.benchmark/benchmark_after_redis.txt (1)
1-233: Consider excluding benchmark artifacts from version control.Benchmark report files should generally not be committed to the repository, as they:
- Are generated outputs, not source code
- Inflate repository size unnecessarily
- Create noise in commit histories
- Make it harder to review actual code changes
- Are better stored in an artifact system (e.g., S3, benchmark dashboard, or CI/CD artifact storage)
Recommend adding these files to
.gitignoreand storing benchmark reports in a dedicated artifact/metrics storage system instead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
benchmark/benchmark_after.txt(1 hunks)benchmark/benchmark_after_redis.txt(1 hunks)benchmark/benchmark_after_v2.txt(1 hunks)benchmark/benchmark_after_v3.txt(1 hunks)benchmark/benchmark_before.txt(1 hunks)benchmark/benchmark_before_redis.txt(1 hunks)benchmark/benchmark_before_v2.txt(1 hunks)package.json(1 hunks)src/api/v2/resolvers/generic.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/api/v2/resolvers/generic.ts (2)
src/api/v2/resolvers/dpid.ts (1)
resolveDpid(129-273)src/util/manifests.ts (1)
getManifest(10-31)
🔇 Additional comments (1)
src/api/v2/resolvers/generic.ts (1)
185-220: [Rewritten review comment]
[Classification tag]
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
src/api/v2/resolvers/generic.ts (3)
134-183: Past review concerns still unaddressed.Previous reviews flagged:
- Missing
Varyheaders for Accept and User-Agent (line 171) - caches/CDNs may serve wrong representationssetHeader("Link", ...)overwrites existing Link headers (line 171) - useappendinsteadSee past review comments for detailed fixes.
338-474: Past critical XSS review concern still unaddressed.Previous review flagged script injection vulnerabilities in HTML generation (lines 419-466):
- Line 423, 437, 442:
manifest.titleandmanifest.descriptioninserted without escaping- Line 432: JSON-LD embedded in
<script>tag without escaping<charactersSee past review comment for detailed XSS fix with proper escapeHtml function.
409-409: Include version in Nodes URL for landing page.The
nodesUrlat line 409 always points to the unversioned URL, even whenversionIxis defined. This inconsistency was noted in a past review comment.Apply the same version logic as the redirect path (lines 480-483):
- const nodesUrl = `${NODES_URL}/dpid/${dpid}`; + let nodesUrl = `${NODES_URL}/dpid/${dpid}`; + if (versionIx !== undefined) { + nodesUrl += `/v${versionIx + 1}`; + }This ensures the "View full Research Object" link takes users to the requested version.
🧹 Nitpick comments (4)
src/api/v2/resolvers/generic.ts (4)
175-183: Extract duplicate license mapping to shared constant.
LICENSES_TO_URLis duplicated fromRoCrateTransformer(mentioned in comment at line 174). This creates maintenance risk if license URLs need updating.Extract to a shared constant file:
// src/util/licenses.ts export const LICENSES_TO_URL: Record<string, string> = { 'CC-BY-4.0': 'https://creativecommons.org/licenses/by/4.0/', 'CC-BY-SA-4.0': 'https://creativecommons.org/licenses/by-sa/4.0/', // ... rest };Then import in both files to maintain a single source of truth.
200-206: Clarify version selection logic.The fallback chain
resolveResult.versions[targetVersionIdx] ?? resolveResult.versions[0]is confusing:
- If
versionIxis undefined,targetVersionIdx = 0(oldest version)- If
versionIxis provided,targetVersionIdx = versionIx- The
?? resolveResult.versions[0]fallback would only trigger if the array access returns undefinedSimplify to make intent clearer:
- const targetVersionIdx = versionIx ?? 0; - const targetVersion = resolveResult.versions[targetVersionIdx] ?? resolveResult.versions[0]; - datePublished = targetVersion?.time; + // Use requested version's timestamp, or earliest version if unspecified + const versionIdx = versionIx ?? 0; + const version = resolveResult.versions[versionIdx]; + datePublished = version?.time;The double-fallback seems unnecessary since
resolveDpidvalidates version bounds.
212-213: Handle missing 'root' component gracefully.If no component named 'root' exists,
rootComponentwill beundefinedand the file size collection silently skips (caught by try-catch at line 230). However, this might indicate a malformed manifest.Consider logging when root component is missing for debugging:
const rootComponent = manifest.components.find(c => c.name === 'root'); if (rootComponent?.payload?.cid) { // ... fetch sizes ... + } else { + logger.info({ dpid, componentNames: manifest.components.map(c => c.name) }, + "No 'root' component found for file size collection"); }
342-351: Simplify user agent detection.Multiple
includes()checks are verbose and harder to maintain when adding new crawlers.Use an array and
some()for cleaner code:- const isCrawlerOrAssessment = - userAgent.includes("F-UJI") || - userAgent.includes("Googlebot") || - userAgent.includes("bingbot") || - userAgent.includes("Slurp") || - userAgent.includes("DuckDuckBot") || - userAgent.includes("facebookexternalhit") || - userAgent.includes("LinkedInBot") || - userAgent.includes("Twitterbot") || - userAgent.includes("Semanticbot"); + const CRAWLER_USER_AGENTS = [ + "F-UJI", "Googlebot", "bingbot", "Slurp", "DuckDuckBot", + "facebookexternalhit", "LinkedInBot", "Twitterbot", "Semanticbot" + ]; + const isCrawlerOrAssessment = CRAWLER_USER_AGENTS.some(ua => userAgent.includes(ua));Alternatively, use a regex for case-insensitive matching:
/F-UJI|Googlebot|bingbot|.../i.test(userAgent).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
package.json(1 hunks)src/api/v2/resolvers/generic.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
🧬 Code graph analysis (1)
src/api/v2/resolvers/generic.ts (3)
src/api/v2/resolvers/dpid.ts (1)
resolveDpid(129-273)src/util/manifests.ts (1)
getManifest(10-31)src/api/v2/data/getIpfsFolder.ts (1)
getIpfsFolderTreeByCid(215-368)
…sting headers for FAIR compliance - Updated package.json to reference local desci-models. - Improved content negotiation in the generic API handler to support JSON-LD and RDF formats via Accept headers. - Added Signposting headers to responses for better metadata discovery and compliance with FAIR principles. - Implemented a landing page for crawlers and assessment tools, embedding JSON-LD and providing relevant links. (required for FUJI)
…AIR compliance - Updated package.json to reference local desci-models. (temporary until nodes PR) - Modified pnpm-lock.yaml to reflect the local dependency change. (temporary until nodes PR) - Implemented functionality in the generic API handler to fetch file sizes from IPFS, improving metadata for FAIR compliance. - Enhanced the landing page with structured metadata and improved accessibility for crawlers and assessment tools.
…adata - Introduced a new function to fetch AI-generated keywords from the nodes API when no keywords are present in the manifest. - Updated the generic API handler to utilize the fetched keywords, improving metadata quality for responses. - Added a utility function to retrieve the nodes API URL based on the environment configuration.
- Upgraded lockfile version to 3 in package-lock.json. - Updated @desci-labs/desci-models dependency to version ^0.2.29-rc1 in package.json. - Added caching functionality for file size retrieval in the generic API handler to improve performance and reduce redundant IPFS calls. - Enhanced error handling and logging for better traceability during file size fetching.
yradwan147
force-pushed
the
FAIRCompliance-yousef
branch
from
d798b12 to
e67a227
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
benchmark/benchmark_after_v3.txt (1)
1-234: Benchmark artifact looks good.This is a well-structured performance report consistent with the existing benchmark series. No functional code is changed. One observation worth noting for operational awareness:
/api/v2/query/objectsshows severe concurrent degradation — sequential avg ~1936ms vs. concurrent avg ~10458ms with a P95 of ~18904ms — indicating it does not scale well under parallel load and may warrant investigation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benchmark/benchmark_after_v3.txt` around lines 1 - 234, The concurrent benchmark shows /api/v2/query/objects suffering extreme degradation (sequential avg ~1936ms vs concurrent avg ~10458ms); locate the handler registered for "/api/v2/query/objects" (e.g., QueryController.getObjects, queryObjectsHandler, or the router entry that binds that path) and profile it to find the blocking hotspot (DB queries, heavy in-memory work, or synchronous loops). Fix by: 1) adding pagination/limits to the endpoint and enforcing sane defaults, 2) optimizing the DB access in the handler (restrict selected columns, add/verify indexes on used filters, use batched queries), 3) introducing caching or a concurrency limiter (semaphore/queue) around the expensive work in queryObjectsHandler/QueryController.getObjects, and 4) add instrumentation (timers/metrics) and a benchmark/regression test for "/api/v2/query/objects" to validate improvements.src/api/v2/resolvers/generic.ts (3)
317-325: Multiple Prettier formatting violations flagged by ESLint.Static analysis reports ~15 formatting errors across this file (single vs. double quotes in
LICENSES_TO_URL, trailing whitespace on lines 264/473-475, multiline formatting in several blocks). Runnpx prettier --write src/api/v2/resolvers/generic.tsto auto-fix all of them.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/v2/resolvers/generic.ts` around lines 317 - 325, ESLint flagged multiple Prettier formatting issues in this file (e.g., inconsistent quotes in LICENSES_TO_URL and stray trailing whitespace); run the auto-formatter and commit the fixes by running npx prettier --write src/api/v2/resolvers/generic.ts (or project-wide npx prettier --write .), then verify LICENSES_TO_URL remains a correct mapping (symbol: LICENSES_TO_URL) and that no trailing whitespace or quote-style inconsistencies remain in generic.ts after formatting.
469-482: User-agent matching is case-sensitive — consider case-insensitive comparison.If any crawler sends a slightly different casing (e.g.,
googlebotinstead ofGooglebot), it won't match. A simple.toLowerCase()onuserAgentbefore comparison would be more robust.♻️ Proposed fix
const userAgent = req.headers["user-agent"] || ""; + const userAgentLower = userAgent.toLowerCase(); const isCrawlerOrAssessment = - userAgent.includes("F-UJI") || - userAgent.includes("Googlebot") || - userAgent.includes("bingbot") || - userAgent.includes("Slurp") || - userAgent.includes("DuckDuckBot") || - userAgent.includes("facebookexternalhit") || - userAgent.includes("LinkedInBot") || - userAgent.includes("Twitterbot") || - userAgent.includes("Semanticbot"); + userAgentLower.includes("f-uji") || + userAgentLower.includes("googlebot") || + userAgentLower.includes("bingbot") || + userAgentLower.includes("slurp") || + userAgentLower.includes("duckduckbot") || + userAgentLower.includes("facebookexternalhit") || + userAgentLower.includes("linkedinbot") || + userAgentLower.includes("twitterbot") || + userAgentLower.includes("semanticbot");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/v2/resolvers/generic.ts` around lines 469 - 482, The user-agent comparisons in the isCrawlerOrAssessment check are case-sensitive; change how userAgent is obtained and compared by normalizing it to lowercase (e.g., set userAgent from req.headers["user-agent"] to a lowercase string) and update all include checks to use lowercase literals (referencing the userAgent variable and the isCrawlerOrAssessment expression) so matches like "googlebot" or "Googlebot" both succeed.
316-325:LICENSES_TO_URLduplication could be resolved by coordinating with@desci-labs/desci-modelsmaintainers.The mapping at lines 316–325 is currently duplicated from
RoCrateTransformer(an external package). If the external package maintainers can export this mapping, importing it would eliminate the maintenance burden. Until then, the existing comment adequately flags the duplication. Consider opening an issue with@desci-labs/desci-modelsto request exporting this mapping.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/v2/resolvers/generic.ts` around lines 316 - 325, Duplicate LICENSES_TO_URL mapping: replace the hard-coded LICENSES_TO_URL constant with an imported export from RoCrateTransformer/@desci-labs/desci-models once maintainers expose it (i.e., remove the local const LICENSES_TO_URL and add "import { LICENSES_TO_URL } from '@desci-labs/desci-models'"), and until that is available, add a TODO comment referencing RoCrateTransformer and open an issue with `@desci-labs/desci-models` to request exporting the mapping so future code can import the single source of truth.benchmark/benchmark_after_redis.txt (1)
1-234: Consider not committing benchmark reports as source-controlled files.Static benchmark outputs are ephemeral and grow stale quickly. They also add diff noise to PRs. Consider storing them as CI artifacts, in a wiki, or behind a dedicated benchmark automation tool (e.g.,
github-action-benchmark) that tracks trends over time without polluting the repo.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benchmark/benchmark_after_redis.txt` around lines 1 - 234, The file benchmark/benchmark_after_redis.txt is a committed static benchmark report that will cause churn and stale data; remove it from the repository and stop committing such outputs by deleting benchmark/benchmark_after_redis.txt from the branch and adding the benchmark output pattern (e.g., benchmark/*.txt or the specific filename) to .gitignore; instead wire your benchmark job to upload the report as a CI artifact or store it in a dedicated benchmarking tool (e.g., github-action-benchmark) so results are preserved without polluting the repo.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/api/v2/resolvers/generic.ts`:
- Around line 539-541: The href attribute uses licenseUrl (computed from
LICENSES_TO_URL or manifest.defaultLicense) unescaped, allowing XSS if
manifest.defaultLicense contains quotes; update the rendering logic around
licenseUrl in generic.ts so you either validate it startsWith("http") before
inserting into the <a href="..."> (only render as a clickable link when safe) or
properly escape/encode the URL for attribute context, and ensure the fallback
renders plain escaped text (manifest.defaultLicense) when the URL is not a safe
absolute http/https URL.
- Around line 260-263: The Signposting `describedby` URL is built without the
version context; update the URL construction so jsonldUrl includes the requested
version when present: modify the base URL logic (symbols: baseUrl, dpidUrl,
jsonldUrl) to incorporate versionIx (the same version token used for nodesUrl
and passed to prepareRoCrateMetadata) — e.g., when versionIx is defined, build
dpidUrl as `${baseUrl}/${versionIx}/${dpid}` (otherwise keep
`${baseUrl}/${dpid}`) and then derive jsonldUrl from that (jsonldUrl =
`${dpidUrl}?format=jsonld`) so both `cite-as` and `describedby` point to the
same versioned JSON‑LD endpoint.
---
Nitpick comments:
In `@benchmark/benchmark_after_redis.txt`:
- Around line 1-234: The file benchmark/benchmark_after_redis.txt is a committed
static benchmark report that will cause churn and stale data; remove it from the
repository and stop committing such outputs by deleting
benchmark/benchmark_after_redis.txt from the branch and adding the benchmark
output pattern (e.g., benchmark/*.txt or the specific filename) to .gitignore;
instead wire your benchmark job to upload the report as a CI artifact or store
it in a dedicated benchmarking tool (e.g., github-action-benchmark) so results
are preserved without polluting the repo.
In `@benchmark/benchmark_after_v3.txt`:
- Around line 1-234: The concurrent benchmark shows /api/v2/query/objects
suffering extreme degradation (sequential avg ~1936ms vs concurrent avg
~10458ms); locate the handler registered for "/api/v2/query/objects" (e.g.,
QueryController.getObjects, queryObjectsHandler, or the router entry that binds
that path) and profile it to find the blocking hotspot (DB queries, heavy
in-memory work, or synchronous loops). Fix by: 1) adding pagination/limits to
the endpoint and enforcing sane defaults, 2) optimizing the DB access in the
handler (restrict selected columns, add/verify indexes on used filters, use
batched queries), 3) introducing caching or a concurrency limiter
(semaphore/queue) around the expensive work in
queryObjectsHandler/QueryController.getObjects, and 4) add instrumentation
(timers/metrics) and a benchmark/regression test for "/api/v2/query/objects" to
validate improvements.
In `@src/api/v2/resolvers/generic.ts`:
- Around line 317-325: ESLint flagged multiple Prettier formatting issues in
this file (e.g., inconsistent quotes in LICENSES_TO_URL and stray trailing
whitespace); run the auto-formatter and commit the fixes by running npx prettier
--write src/api/v2/resolvers/generic.ts (or project-wide npx prettier --write
.), then verify LICENSES_TO_URL remains a correct mapping (symbol:
LICENSES_TO_URL) and that no trailing whitespace or quote-style inconsistencies
remain in generic.ts after formatting.
- Around line 469-482: The user-agent comparisons in the isCrawlerOrAssessment
check are case-sensitive; change how userAgent is obtained and compared by
normalizing it to lowercase (e.g., set userAgent from req.headers["user-agent"]
to a lowercase string) and update all include checks to use lowercase literals
(referencing the userAgent variable and the isCrawlerOrAssessment expression) so
matches like "googlebot" or "Googlebot" both succeed.
- Around line 316-325: Duplicate LICENSES_TO_URL mapping: replace the hard-coded
LICENSES_TO_URL constant with an imported export from
RoCrateTransformer/@desci-labs/desci-models once maintainers expose it (i.e.,
remove the local const LICENSES_TO_URL and add "import { LICENSES_TO_URL } from
'@desci-labs/desci-models'"), and until that is available, add a TODO comment
referencing RoCrateTransformer and open an issue with `@desci-labs/desci-models`
to request exporting the mapping so future code can import the single source of
truth.
| // Build base URLs for Signposting headers | ||
| const baseUrl = `${req.protocol}://${req.get("host")}`; | ||
| const dpidUrl = `${baseUrl}/${dpid}`; | ||
| const jsonldUrl = `${dpidUrl}?format=jsonld`; |
There was a problem hiding this comment.
Signposting describedby link loses version context for versioned requests.
jsonldUrl is always ${dpidUrl}?format=jsonld without a version segment. When a crawler requests e.g. /46/v2, the landing page correctly uses versionIx for nodesUrl (line 498) and passes it to prepareRoCrateMetadata (line 492), but the describedby Signposting link directs assessment tools to the unversioned JSON-LD endpoint, which may return metadata for a different version.
For FAIR compliance, the describedby link should match the version being served.
🛡️ Proposed fix
const baseUrl = `${req.protocol}://${req.get("host")}`;
- const dpidUrl = `${baseUrl}/${dpid}`;
- const jsonldUrl = `${dpidUrl}?format=jsonld`;
+ const dpidUrl = versionIx !== undefined
+ ? `${baseUrl}/${dpid}/v${versionIx + 1}`
+ : `${baseUrl}/${dpid}`;
+ const jsonldUrl = `${dpidUrl}?format=jsonld`;This ensures both cite-as and describedby include the version when a specific version was requested.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api/v2/resolvers/generic.ts` around lines 260 - 263, The Signposting
`describedby` URL is built without the version context; update the URL
construction so jsonldUrl includes the requested version when present: modify
the base URL logic (symbols: baseUrl, dpidUrl, jsonldUrl) to incorporate
versionIx (the same version token used for nodesUrl and passed to
prepareRoCrateMetadata) — e.g., when versionIx is defined, build dpidUrl as
`${baseUrl}/${versionIx}/${dpid}` (otherwise keep `${baseUrl}/${dpid}`) and then
derive jsonldUrl from that (jsonldUrl = `${dpidUrl}?format=jsonld`) so both
`cite-as` and `describedby` point to the same versioned JSON‑LD endpoint.
| ${authorNames ? `<dt>Authors</dt><dd itemprop="creator">${authorNames}</dd>` : ""} | ||
| <dt>License</dt> | ||
| <dd><a href="${licenseUrl}" itemprop="license">${escapeHtml(manifest.defaultLicense || "See license")}</a></dd> |
There was a problem hiding this comment.
Unescaped licenseUrl in href attribute — potential XSS.
licenseUrl at line 541 can fall through to the raw manifest.defaultLicense value (line 500) when it's not found in LICENSES_TO_URL. Unlike line 519 which guards with startsWith("http"), line 541 unconditionally embeds it in an <a href="..."> attribute. A crafted defaultLicense value containing " could break out of the attribute.
🛡️ Proposed fix
<dt>License</dt>
- <dd><a href="${licenseUrl}" itemprop="license">${escapeHtml(manifest.defaultLicense || "See license")}</a></dd>
+ <dd><a href="${escapeHtml(licenseUrl)}" itemprop="license">${escapeHtml(manifest.defaultLicense || "See license")}</a></dd>Alternatively, apply the same startsWith("http") guard used at line 519 to only render a clickable link when the URL is valid:
<dt>License</dt>
- <dd><a href="${licenseUrl}" itemprop="license">${escapeHtml(manifest.defaultLicense || "See license")}</a></dd>
+ <dd>${licenseUrl.startsWith("http")
+ ? `<a href="${escapeHtml(licenseUrl)}" itemprop="license">${escapeHtml(manifest.defaultLicense || "See license")}</a>`
+ : `<span itemprop="license">${escapeHtml(manifest.defaultLicense || "See license")}</span>`}</dd>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api/v2/resolvers/generic.ts` around lines 539 - 541, The href attribute
uses licenseUrl (computed from LICENSES_TO_URL or manifest.defaultLicense)
unescaped, allowing XSS if manifest.defaultLicense contains quotes; update the
rendering logic around licenseUrl in generic.ts so you either validate it
startsWith("http") before inserting into the <a href="..."> (only render as a
clickable link when safe) or properly escape/encode the URL for attribute
context, and ensure the fallback renders plain escaped text
(manifest.defaultLicense) when the URL is not a safe absolute http/https URL.
- Escape licenseUrl in HTML href attributes (XSS hardening): only render as clickable link when URL starts with http(s), use encodeURI for attribute context, fallback to plain escaped text otherwise - Include version in Signposting describedby/cite-as URLs so they point to the versioned JSON-LD endpoint when a version is requested - Normalize user-agent to lowercase for case-insensitive crawler matching - Fix quote style inconsistencies (prettier) and add TODO for importing LICENSES_TO_URL from desci-models once exported - Remove committed benchmark/*.txt outputs and add pattern to .gitignore Co-authored-by: Cursor <cursoragent@cursor.com>
1 participant
Summary by CodeRabbit
New Features
Chores