Update buildUrl to prepend webUrlPrefix

[Wolframcheg]

Pull Request Template

Description

Update buildUrl to prepend webUrlPrefix

Type of Change

Bug fix (non-breaking change which fixes an issue)

[pchuri]

Thanks for the PR! The intent behind this fix makes sense — attachment download links are missing the /wiki prefix on instances that use it.

However, I have a concern with modifying toAbsoluteUrl() directly. Since it's a general-purpose utility method, adding webUrlPrefix inside it could cause double-prefixing if the input path already contains /wiki, and may introduce unintended side effects for future callers.

The rest of the codebase handles this at the call site (e.g., lines 416, 510, 1356), explicitly passing webUrlPrefix to buildUrl(). I'd suggest keeping toAbsoluteUrl() unchanged and applying the prefix at the two call sites instead:

// normalizeAttachment (line 2063)
downloadLink: this.toAbsoluteUrl(
  raw._links?.download ? `${this.webUrlPrefix}${raw._links.download}` : null
)

// downloadAttachment (line 864)
downloadUrl = this.toAbsoluteUrl(
  attachment._links?.download ? `${this.webUrlPrefix}${attachment._links.download}` : null
);

This keeps the fix scoped to where it's needed and stays consistent with the existing patterns. What do you think?

[Wolframcheg]

I think your option is better

[adamtan945]

Independently hit this same bug and opened #99 before seeing this PR — apologies for the duplication. Closing #99 in favor of this one since it was opened first. Sharing my root cause analysis and tests here so they're useful for the review.

Root cause

The REST API returns each attachment's _links.download as a path relative to the Confluence context root:

/download/attachments/123456789/example.png?version=1&modificationDate=1700000000000&cacheVersion=1&api=v2

On Atlassian Cloud the context root is /wiki, so the correct URL is https://<domain>/wiki/download/.... Without this patch, toAbsoluteUrl hands the raw path to buildUrl, producing https://<domain>/download/... — which returns 404 on every Cloud instance. That's what breaks confluence attachments --download completely on Cloud (issue #98).

The fix here is correct: prepend webUrlPrefix, which is already computed on line 38 (this.apiPath.startsWith('/wiki/') ? '/wiki' : '') and is an empty string on Server/DC, so this change is backward-compatible.

Verified by curl'ing the two URLs directly against a live Cloud instance with the same credentials:

URL	Result
https://<domain>/download/attachments/.../?version=... (before fix)	HTTP 404
https://<domain>/wiki/download/attachments/.../?version=... (with this fix)	HTTP 200

End-to-end verification

Checked this branch out locally, ran npm install && npm link, then against a real Cloud page:

$ confluence attachments <pageId> --download --dest /tmp/out
⬇️  example.png -> /tmp/out/example.png
Downloaded 1 attachment to /tmp/out

The downloaded file is byte-identical to what you get by fetching _links.download directly with curl against the REST API.

Suggested tests (cherry-pick-able)

I wrote three targeted tests while working on #99 that would pin this fix down. Happy to send them as a follow-up PR if you'd prefer, or you can copy them in here — they're small:

test('toAbsoluteUrl prepends /wiki context path on Atlassian Cloud', () => {
  const cloudClient = new ConfluenceClient({
    domain: 'test.atlassian.net',
    token: 'token',
    apiPath: '/wiki/rest/api'
  });
  expect(cloudClient.toAbsoluteUrl('/download/attachments/123/file.png?version=1&modificationDate=1700000000000&cacheVersion=1&api=v2'))
    .toBe('https://test.atlassian.net/wiki/download/attachments/123/file.png?version=1&modificationDate=1700000000000&cacheVersion=1&api=v2');
});

test('toAbsoluteUrl does not double-prepend /wiki when path already starts with it', () => {
  const cloudClient = new ConfluenceClient({
    domain: 'test.atlassian.net',
    token: 'token',
    apiPath: '/wiki/rest/api'
  });
  expect(cloudClient.toAbsoluteUrl('/wiki/download/attachments/123/file.png'))
    .toBe('https://test.atlassian.net/wiki/download/attachments/123/file.png');
});

test('toAbsoluteUrl leaves Server/DC paths untouched (no webUrlPrefix)', () => {
  const serverClient = new ConfluenceClient({
    domain: 'confluence.example.com',
    token: 'token',
    apiPath: '/rest/api'
  });
  expect(serverClient.toAbsoluteUrl('/download/attachments/123/file.png'))
    .toBe('https://confluence.example.com/download/attachments/123/file.png');
});

One optional hardening

The current one-liner has no guard against _links.download ever starting with /wiki already (it doesn't today, but a future API change could cause /wiki/wiki/...). If you want to be defensive:

const prefix = this.webUrlPrefix || '';
const needsPrefix = prefix && !pathOrUrl.startsWith(`${prefix}/`);
return this.buildUrl(needsPrefix ? `${prefix}${pathOrUrl}` : pathOrUrl);

But the existing one-liner is fine for today's API behavior and matches the style of how webUrlPrefix is used elsewhere in the codebase (lines 416, 510, 1356, 1360 all just concatenate without guards). Totally up to the maintainer.

[pchuri]

Thanks for the fix! This correctly identifies the root cause — the missing webUrlPrefix (/wiki) in toAbsoluteUrl is what causes the 404 on Atlassian Cloud.

One concern: if pathOrUrl already starts with /wiki (e.g., some API responses may return paths that already include it), the current change would produce /wiki/wiki/..., resulting in another 404.

A safer approach would be to guard against duplicate prefixes:

toAbsoluteUrl(pathOrUrl) {
  if (!pathOrUrl) {
    return null;
  }

  if (pathOrUrl.startsWith('http://') || pathOrUrl.startsWith('https://')) {
    return pathOrUrl;
  }

  const pathWithPrefix = this.webUrlPrefix && !pathOrUrl.startsWith(this.webUrlPrefix)
    ? `${this.webUrlPrefix}${pathOrUrl}`
    : pathOrUrl;
  return this.buildUrl(pathWithPrefix);
}

This is consistent with the pattern already used elsewhere in the codebase (lines 416, 510, 1356, 1360) where webUrlPrefix is explicitly prepended.

Also, it would be great to add a test case verifying Cloud download URLs include the /wiki prefix — the current test suite doesn't cover this scenario.

Note: This also relates to #98 — the issue reporter attributed the 404 to missing query parameters (version, modificationDate, cacheVersion), but the actual root cause is the missing /wiki prefix. The query params are already preserved through buildUrl since _links.download includes them.