OpenConceptLab/ocl_online#374 | using path routing in place of hash routing

[snyaggarwal]

Linked Issue

Refs OpenConceptLab/ocl_online#374

oclweb3 PR - OpenConceptLab/oclweb3#61

[paynejd]

Reviewed against OpenConceptLab/ocl_online#374 alongside OpenConceptLab/oclweb3#61. history.js is identical there, so the same suggestions apply; I checked them in a local oclweb3 build, and here lint and all 416 tests pass with them. Codex's review follows.

Before merge

1. Forced logout loses the page's query after its first parameter (utils.js L665).
2. history.js should accept only single-slash routes (L13, L31). Details in OpenConceptLab/ocl_online#374.

Small
3. ?origin=openmrs now redirects from every path (index.html L47).
4. A relative #/… link picks up the current page's query (history.js L61).
5. toFullURL still builds /# URLs but has no callers, so it can go. toV3URL can keep /# until both apps are live; then the comment at ConceptHeader.jsx:43 goes too.

Three of Codex's points don't happen today: no OCL app puts referrer before the #, every sign-in returns to LOGIN_REDIRECT_URL (the site root), and every blob: link has download.

[paynejd]

oclapi2 decodes post_logout_redirect_uri and passes it to Keycloak raw, so Keycloak splits it at each & and the page's query comes back cut at its first &, even if getSSOLogoutURL encodes it (Codex's fix). As a fragment, the pre-PR form, it never reaches oclapi2 or Keycloak, so their redirects carry it through intact; the legacy-hash redirect then turns it into /signin?returnTo=…. Same fix as OpenConceptLab/oclweb3#61, checked there against a mock of both redirects. Keycloak's post-logout URIs then need no change.

Suggested change

	const redirectURL = forced ?
	window.location.origin + '/#/signin?returnTo=' + encodeURIComponent(returnTo) :
	window.location.origin + '/signin?returnTo=' + encodeURIComponent(returnTo) :
	// A fragment never reaches oclapi2 or Keycloak, so it survives their redirects intact (a query is decoded and
	// split on '&'). Back in the app, the legacy-hash redirect turns it into /signin?returnTo=…
	const redirectURL = forced ?
	window.location.origin + '/#/signin?returnTo=' + encodeURIComponent(returnTo) :

[paynejd]

Same-site paths only. Details in OpenConceptLab/ocl_online#374.

Suggested change

	if(!path)
	return
	const routePath = toRoutePath(path)
	const routePath = toRoutePath(path)
	// Same-site paths only: '//host' or '/\host' would leave the site, or make pushState throw.
	if(!/^\/(?![/\\])/.test(routePath || ''))
	return

[paynejd]

Skip #//… and #/\…. Details in OpenConceptLab/ocl_online#374.

Suggested change

	if(!hash.startsWith('#/') || /[?&]referrer=/.test(search))
	if(!hash.startsWith('#/') || /^#\/[/\\]/.test(hash) || /[?&]referrer=/.test(search))

[paynejd]

A relative #/… href resolves against this page, so legacyHashRoute merges this page's query into the route. Also limits routing to http(s) links.

Suggested change

	const url = new URL(anchor.href, window.location.href)
	if(url.origin !== window.location.origin)
	// Resolved against this page, a relative '#/…' link would pick up this page's query.
	if(href.startsWith('#/'))
	return href.slice(1)
	const url = new URL(anchor.href, window.location.href)
	if(url.origin !== window.location.origin || !/^https?:$/.test(url.protocol))

[paynejd]

The Mapper has no /search route, so this branch only ever sent origin=openmrs visitors to a missing page, and since this PR it fires on every path.

Suggested change

	} else if((loc.search || '').includes('origin=openmrs')) {
	go('/search/?origin=openmrs&type=organizations');

[paynejd]

Codex adversarial review, pass 1 (codex-cli 0.160.1, commit 74f85e2)

I found six issues. No files were modified.

1. [P2] Forced logout corrupts returnTo query strings.
   src/common/utils.js:665, src/common/utils.js:955
   The inner returnTo is encoded, but the entire sign-in URL is interpolated into post_logout_redirect_uri without encoding. Parsing the logout request consumes that encoding prematurely. A destination containing multiple query parameters loses parameters when /signin subsequently parses it; nested encoded values also change.
   Verified: reproduced the corruption using the actual helper and URL parsing. Keycloak was not exercised.
   Fix: construct the logout query with URLSearchParams, encoding the complete redirect URI as one parameter.

2. [P2] Legacy redirects skip genuine routes whenever an outer referrer exists.
   src/common/history.js:31
   Opening /?referrer=https%3A%2F%2Fapp.v3.openconceptlab.org%2F#/orgs/OCL/map-projects/123/ returns no legacy route. BrowserRouter consequently selects /; for an authenticated user, forceLoginUser removes the hash and leaves the dashboard instead of the project. The same exception affects legacy returnTo normalization.
   Verified: the helper returns null; the resulting routing follows directly from the surrounding code.
   Fix: distinguish a referrer’s embedded fragment from the primary legacy route. Encode referrer values in newly generated links rather than excluding every URL containing that parameter.

3. [P2] The global interceptor hijacks same-origin non-app links.
   src/common/history.js:61
   Origin equality is the only route boundary. Ordinary links to /favicon.ico, /bootstrap.min.css, or a server export endpoint are prevented and pushed into React Router, producing the app’s 404 instead of fetching the resource. Same-origin blob: URLs are also intercepted and converted incorrectly.
   Verified: synthetic clicks against the actual helper demonstrated each interception. Links carrying download are correctly excluded.
   Fix: require an HTTP(S) URL belonging to an app route, or explicitly opt app links into routing. Preserve native navigation for other resources.

4. [P2] Query-mode OAuth callbacks on legacy non-root paths no longer reach code exchange.
   public/index.html:43
   A sign-in started before the fixed callback change can return to /orgs/OCL/map-projects/123/?state=…&code=…. Previously this was redirected into the callback route with next; the new pathname restriction leaves it on the project route, where nobody exchanges the code. The callback component still explicitly supports these older sign-ins.
   Verified: executing the actual bootstrap script with a non-root callback produces no redirect.
   Fix: recognize supported legacy OAuth callbacks before cleanup, route them to /oidc/login, and preserve the original callback path and redirect URI.

5. [P2] Legacy-route validation. Moved to OpenConceptLab/ocl_online#374 (private).

6. [P3] The live sibling-app URL helper still generates hash URLs.
   src/common/utils.js:1113
   toV3URL still emits https://app.v3…/#/…, affecting menus, entity chips, concept links, and collection links. These target oclweb3, so they do not qualify for the oclweb2 exception. toFullURL also retains /#, although I found no callers. The sibling PR similarly leaves toMapperURL emitting hashes.
   Verified: whole-tree search, caller inspection, and execution of toV3URL.
   Fix: generate clean sibling-app paths and encode referrer parameters; retain hashes specifically for oclweb2.

Verification and limits:

• The nine new history tests pass. The complete unit run reported 378 passes and one test-file load failure, caused by missing lodash. No dependencies were installed.
• Helper checks preserved modifier clicks, _blank, external links, downloads, encoded query values, and dotted IDs. Real React event propagation and back/forward behavior remain unverified.
• Webpack’s absolute publicPath, absolute public assets, development disableDotRule, and nginx’s try_files $uri /index.html look correct for deep links containing dots. The production image was not run. Missing assets still receive the existing HTML fallback.
• The requested main comparison was empty against the local origin/main reference.
• GA’s pathname calculation is updated, but explicit page-view reporting still runs only on App mount; setUpRecentHistory has no callers. Per-navigation reporting depends on GA’s configuration, which I could not verify. This appears preexisting.
• Browser execution required approval unavailable in this session; Keycloak, email verification/reset, and production serving were not tested end to end.

Overall verdict: merge after fixes.