BAH-4789: Fix Form Builder pagination CORS error from absolute next links by anushkadubey-star · Pull Request #141 · Bahmni/implementer-interface

anushkadubey-star · 2026-10-22T12:06:19Z

Ticket

BAH-4789 list page fails to load paginated forms (next link contains http://localhost, causing CORS error)

Root cause

OpenMRS builds links[rel=next] from the proxied request context, so the URL is http://localhost/openmrs/.... The Form Builder followed it as-is, so the browser blocked it as cross-origin and only the first page showed.

Solution

New getNextPageUrl (src/common/utils/paginationUtils.js) selects the rel=next link and keeps only its path and encoded query, so requests go to the current origin.
FormBuilderContainer.collectAllForms uses it and still merges all pages into one list (no UI change).
Failed, invalid and repeated next links show an error and stop; no partial list or infinite loop.
collectAllForms now returns its promise chain, so getFormData() (and componentDidMount's following getDefaultLocale()) waits until all pages are loaded instead of only the first. This is intentional and a small behavior change; the list is still rendered once, after all pages.

Tests performed

Karma (Node 12.22.12): 437/437 passed, including 12 new tests (helper + container).
ESLint: 0 errors (existing warnings only, none introduced).
Webpack build to a temp dir succeeded.
Real browser (headless Chrome) with limit=2 forced: original build gets a CORS error and shows 0 forms; fixed build requests same-origin /openmrs/... pages (all 200) and shows all 26 forms.

…inks OpenMRS builds rel=next links from the proxied request, so they point to http://localhost and are blocked by the browser. Keep only the path and query of the rel=next link so follow-up page requests go to the current origin, and surface errors for failed, invalid or repeated links. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

sahupadma

Review for BAH-4789

Solid, well-tested fix for the root cause (proxy-mismatched origin in OpenMRS's rel=next link), and it incidentally fixes a second latent bug (the old code could grab links[0].uri regardless of rel). One blocking issue: the origin-stripping in getNextPageUrl can itself be bypassed by a protocol-relative pathname, which reintroduces the exact cross-origin behavior this PR is meant to eliminate.

Found 1 blocking issue, 2 suggestions, and 3 nits.

Acceptance Criteria: 1/1 met structurally (pagination completes across N pages without CORS errors), but see the blocking comment below for a bypass in the same mechanism that enforces it.

Issues not in diff

File:Line	Type	Issue
src/form-builder/components/FormPrivilegeTable.jsx:304-313	suggestion	`FormPrivilegeTable.collectAllPrivileges` has its own, separate fix (`getNextPaginationRequestUrl`) for the identical OpenMRS pagination/CORS problem, but it only rewrites `protocol`, not `host` — so it doesn't actually fix the cross-origin scenario and is left unaddressed by this PR. Consider reusing the new `common/utils/paginationUtils.getNextPageUrl` here instead of carrying two divergent implementations.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

Cover protocol-relative and dot-segment next links in getNextPageUrl, and the pagination behaviour of FormPrivilegeTable.collectAllPrivileges. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

sahupadma

Review for BAH-4789

Solid fix for the core CORS bug (getNextPageUrl is carefully done and holds up against several host-confusion vectors beyond the included tests), but FormPrivilegeTable got the URL fix without the matching safety net that was applied to FormBuilderContainer.

Found 2 blocking issues, 2 suggestions, 1 nit, and 2 questions.

Acceptance Criteria: Core pagination fix is met everywhere; the "failed/invalid/repeated next-link shows an error and stops" guarantee is met in FormBuilderContainer but not in FormPrivilegeTable (see blocking comments).

Full local review: PR_141_code_review.md in the repo.

sahupadma · 2026-10-26T10:13:18Z

-        return;
-      }
-      const nextUri = this.getNextPaginationRequestUrl(nextLink.uri);
+    const nextUri = getNextPageUrl(initialPrivileges);


blocking: This pagination chain has no .catch anywhere — not in fetchPrivileges() (lines 94-103), nor here. getNextPageUrl throws synchronously for an invalid/malformed next link, and since that throw happens inside an unguarded .then(), it becomes an unhandled promise rejection: no notification is shown, and loading stays true forever (stuck spinner).

Mirror FormBuilderContainer.getFormData()'s pattern: have collectAllPrivileges return its httpInterceptor.get(...).then(...) chain, and add a .catch((error) => { this.setErrorMessage(error); this.setState({ loading: false }); }) in fetchPrivileges().

sahupadma · 2026-10-26T10:13:18Z

-      }
-      const nextUri = this.getNextPaginationRequestUrl(nextLink.uri);
+    const nextUri = getNextPageUrl(initialPrivileges);
+    if (nextUri) {


blocking: Unlike FormBuilderContainer.collectAllForms (which now tracks requestedUrls and throws on a repeated next-link), this method has no protection against a repeated/looping next link — the exact OpenMRS proxy-URL bug class this PR fixes. The only incidental stop is allPrivileges.length === formBuilderConstants.dataLimit (9999, strict equality), which won't trigger if the page size doesn't evenly divide 9999 — making this an effectively unbounded request loop in that case.

Thread the same requestedUrls guard used in FormBuilderContainer.collectAllForms through here.

sahupadma · 2026-10-26T10:13:18Z

+// (e.g. http://localhost/openmrs/...), which need not match the origin of the browser.
+// Only the path and query of the `next` link are kept so that the request is made to the
+// current origin. Returns undefined when there is no next page and throws for an invalid link.
+export function getNextPageUrl(response) {


suggestion: Both collectAllForms and collectAllPrivileges duplicate the same "push results → check limit → getNextPageUrl → fetch next → recurse" loop. Only the URL computation was extracted into this shared util, which is likely why the two copies have already drifted (one has catch + loop-guard, the other doesn't — see comments on FormPrivilegeTable.jsx).

Consider extracting a shared "fetch all pages" helper here that handles accumulation, getNextPageUrl, repeated-link detection, and error propagation, so both call sites can't diverge again.

sahupadma · 2026-10-26T10:13:18Z

+      results: [{ name: 'a' }],
+      links: [{ rel: 'next', uri: 'http://localhost//openmrs/ws/rest/v1/privilege?limit=1' }],
+    };
+    expect(() => wrapper.instance().collectAllPrivileges(first, [])).to.throw('Invalid pagination');


suggestion: This calls collectAllPrivileges directly rather than through the real fetchPrivileges() entry point, so it doesn't exercise (and in fact masks) the missing .catch — in production this throw happens inside an unawaited .then(), not synchronously like it does here.

Consider adding a test that drives through fetchPrivileges() with an invalid/repeating next link stubbed on httpInterceptor.get, asserting on wrapper.state('notification')/wrapper.state('loading'), mirroring FormBuilderContainer.spec.js's coverage for the equivalent scenarios.

sahupadma · 2026-10-26T10:13:18Z

-      .then((initialForms) => {
-        this.collectAllForms(initialForms, forms);
-      })
+      .then((initialForms) => this.collectAllForms(initialForms, forms))


nit: requestedUrls starts empty here, so it never includes the initial fetchFormsUrl. A next link that cycled back to page 1's exact URL wouldn't be caught by this guard (though any further repeat would be). Low-likelihood given real OpenMRS URL formats differ between the initial and next requests, but could seed requestedUrls with the initial URL for completeness.

sahupadma · 2026-10-26T10:13:18Z

+      .then((nextForms) => this.collectAllForms(nextForms, forms, [...requestedUrls, nextUrl]));
  }

  getDefaultLocale() {


question: getFormData/collectAllForms were specifically reworked in this PR to return their promise chains so componentDidMount can .then() off full completion. Should getDefaultLocale() follow the same pattern (return its promise) for consistency, even though nothing currently awaits it?

sahupadma · 2026-10-26T10:13:18Z

+    throw new Error(`Invalid pagination link: ${nextLink.uri}`);
+  }
+  // A path starting with two slashes would be read as a network-path reference to another host
+  if ((nextUrl.protocol !== 'http:' && nextUrl.protocol !== 'https:')


question: This host-confusion guard works correctly against the dot-segment/backslash/triple-slash vectors I tried, but only because host/origin are discarded entirely and the URL parser normalizes backslashes/dot-segments before pathname is read here. Worth a one-line comment calling that invariant out explicitly, so a future refactor doesn't swap in nextUrl.href/host and silently reopen this?

sahupadma reviewed Oct 23, 2026

View reviewed changes

Comment thread src/common/utils/paginationUtils.js

Comment thread test/common/utils/paginationUtils.spec.js

Comment thread src/common/utils/paginationUtils.js Outdated

Comment thread src/form-builder/components/FormBuilderContainer.jsx

anushkadubey-star and others added 2 commits October 23, 2026 14:28

BAH-4789: Address review comments on pagination link handling

3be21be

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

BAH-4789: Add regression tests for pagination link handling

7077130

Cover protocol-relative and dot-segment next links in getNextPageUrl, and the pagination behaviour of FormPrivilegeTable.collectAllPrivileges. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

sahupadma approved these changes Oct 26, 2026

View reviewed changes

anushkadubey-star merged commit 6f4a16b into master Oct 26, 2026
8 checks passed

anushkadubey-star deleted the BAH-4789-form-builder-pagination branch October 26, 2026 07:09

anushkadubey-star mentioned this pull request Oct 26, 2026

BAH-4789: Harden FormPrivilegeTable pagination error handling #142

Open

sahupadma reviewed Oct 26, 2026

View reviewed changes

Navigation Menu

Repository navigation

BAH-4789: Fix Form Builder pagination CORS error from absolute next links - #141

BAH-4789: Fix Form Builder pagination CORS error from absolute next links#141
anushkadubey-star merged 3 commits into
masterfrom
BAH-4789-form-builder-pagination

anushkadubey-star commented Oct 22, 2026 •

edited

Loading

Uh oh!

sahupadma left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sahupadma left a comment

Uh oh!

sahupadma Oct 26, 2026

Uh oh!

sahupadma Oct 26, 2026

Uh oh!

sahupadma Oct 26, 2026

Uh oh!

sahupadma Oct 26, 2026

Uh oh!

sahupadma Oct 26, 2026

Uh oh!

sahupadma Oct 26, 2026

Uh oh!

sahupadma Oct 26, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

anushkadubey-star commented Oct 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Ticket

Root cause

Solution

Tests performed

Uh oh!

sahupadma left a comment

Choose a reason for hiding this comment

Review for BAH-4789

Issues not in diff

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sahupadma left a comment

Choose a reason for hiding this comment

Review for BAH-4789

Uh oh!

sahupadma Oct 26, 2026

Choose a reason for hiding this comment

Uh oh!

sahupadma Oct 26, 2026

Choose a reason for hiding this comment

Uh oh!

sahupadma Oct 26, 2026

Choose a reason for hiding this comment

Uh oh!

sahupadma Oct 26, 2026

Choose a reason for hiding this comment

Uh oh!

sahupadma Oct 26, 2026

Choose a reason for hiding this comment

Uh oh!

sahupadma Oct 26, 2026

Choose a reason for hiding this comment

Uh oh!

sahupadma Oct 26, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

anushkadubey-star commented Oct 22, 2026 •

edited

Loading