[Security Advisory Patches Portal]: Initial Release#677
Open
Suwagath wants to merge 59 commits into
Open
Conversation
… components, including configuration files, service implementations, and UI elements.
Removed log statement for service startup.
… text in the layout component.
…le updates and memory leaks. Added mounted state check to ensure state updates only occur if the component is still mounted, and cleaned up object URLs on unmount.
…mapping for file extensions, improving maintainability and adding case-insensitive matching.
…ates. Added error display for download failures and an empty state message when no preview URL is available. Updated button disable logic to account for download errors.
… Portal' if not provided in window.config
…ement's existence before rendering the application.
…te/trailing slashes, and ensure currentPath ends with a single '/'.
…err CodeRabbit's suggestion.
…ed only once, improving request handling for unauthenticated requests.
…tor to streamline request configuration.
…R for improved consistency and maintainability.
…cation interceptor ID, preventing duplicate interceptors and ensuring proper cleanup during retries.
…plify error message handling in service.bal for improved clarity.
…try-axios for improved error handling, and configure retry behavior with token refresh during retry attempts.
…tively by using a ref for the preview URL, preventing memory leaks on unmount.
…utils.bal Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…t.tsx Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Agzaiyenth Ganaraj <agzaiyenth2@gmail.com>
…constants.bal Co-authored-by: Agzaiyenth Ganaraj <agzaiyenth2@gmail.com>
…utils.bal Co-authored-by: Agzaiyenth Ganaraj <agzaiyenth2@gmail.com>
Co-authored-by: Agzaiyenth Ganaraj <agzaiyenth2@gmail.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Contributor
Author
|
@coderabbitai can you do a full review? I have simplified the app a lot as the requirement got changed. |
Contributor
|
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 18
🧹 Nitpick comments (4)
apps/security-advisory-patches-portal/webapp/config-overrides.js (1)
18-22: ⚡ Quick winNarrow dev-server allowed hosts instead of using
'all'.
allowedHosts: 'all'is overly permissive. Prefer an explicit allowlist (e.g.,localhost,127.0.0.1,patches.wso2.com) to reduce accidental exposure during local/dev-network usage.🔧 Suggested change
if (env === 'development' && config.devServer) { config.devServer = { ...config.devServer, - allowedHosts: 'all', + allowedHosts: ['localhost', '127.0.0.1', 'patches.wso2.com'], }; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/security-advisory-patches-portal/webapp/config-overrides.js` around lines 18 - 22, The dev-server currently sets config.devServer.allowedHosts = 'all' which is too permissive; update the block that runs when env === 'development' to replace the string 'all' with an explicit allowlist array (e.g., ['localhost', '127.0.0.1', 'patches.wso2.com']) so config.devServer.allowedHosts is an array of permitted hostnames; modify the code near the config.devServer assignment to use that array (or read the list from an environment variable and fallback to the explicit list) to narrow allowed hosts.apps/security-advisory-patches-portal/webapp/.gitignore (1)
3-3: ⚡ Quick winTrack lockfile in VCS for reproducible and safer dependency resolution.
Ignoring
package-lock.jsonmakes installs less deterministic across environments and can weaken dependency review/audit consistency. Consider committing it and removing this ignore rule.Proposed change
node_modules/ public/config.js -package-lock.json build/🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/security-advisory-patches-portal/webapp/.gitignore` at line 3, Remove the ignore entry for package-lock.json from .gitignore and add/commit the actual package-lock.json file to the repo so dependency resolution is reproducible; specifically delete the "package-lock.json" line in the .gitignore and run git add/commit for package-lock.json (and push) so the lockfile is tracked in version control.apps/security-advisory-patches-portal/webapp/src/app/AppHandler.tsx (2)
68-96: ⚡ Quick winExtract duplicate loading spinner UI to reduce repetition.
The loading screen at lines 70–95 and the "Opening your link" screen at lines 107–132 share nearly identical structure (CircularProgress, Box layout, flex centering, typography). Extracting this to a reusable component (e.g.,
<LoadingScreen message="..." />) would improve maintainability and ensure consistent styling.♻️ Proposed refactor: Extract shared loading UI
Create a new component:
// src/components/LoadingScreen.tsx import React from 'react'; import { Box, CircularProgress } from '@mui/material'; interface LoadingScreenProps { message: string; } export const LoadingScreen: React.FC<LoadingScreenProps> = ({ message }) => ( <Box sx={{ display: 'flex', flexDirection: 'column', alignItems: 'center', justifyContent: 'center', height: '100vh', bgcolor: '#f5f5f5', gap: 3, }} > <CircularProgress size={60} /> <Box sx={{ textAlign: 'center' }}> <Box component="span" sx={{ fontSize: '1.1rem', color: 'text.secondary', fontWeight: 500, }} > {message} </Box> </Box> </Box> );Then replace both instances in
AppHandler.tsx:+import { LoadingScreen } from '@src/components/LoadingScreen'; if (isLoading || !isAuthenticated) { - return ( - <Box sx={{ ... }}> - <CircularProgress size={60} /> - <Box sx={{ textAlign: 'center' }}> - <Box component="span" sx={{ ... }}> - Getting things ready... - </Box> - </Box> - </Box> - ); + return <LoadingScreen message="Getting things ready..." />; } // ... if (oauthPdfResume) { - return ( - <Box sx={{ ... }}> - <CircularProgress size={60} /> - <Box sx={{ textAlign: 'center' }}> - <Box component="span" sx={{ ... }}> - Opening your link… - </Box> - </Box> - </Box> - ); + return <LoadingScreen message="Opening your link…" />; }Also applies to: 105-133
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/security-advisory-patches-portal/webapp/src/app/AppHandler.tsx` around lines 68 - 96, The duplicated centered loading UI in AppHandler (the isLoading || !isAuthenticated block and the "Opening your link" block) should be extracted into a reusable component (e.g., LoadingScreen) to remove repetition and centralize styling; create a LoadingScreen React component that accepts a message prop and renders the shared Box/CircularProgress/typography structure, then replace the duplicated JSX in AppHandler with <LoadingScreen message="..."/> (use the same sx styles and CircularProgress size) to keep behavior identical.
33-51: 💤 Low valueClarify non-null assertion safety with early validation.
The non-null assertion
navigate(redirectPath!, { replace: true })at line 46 is safe because line 40 ensuresredirectPathOnly(derived fromredirectPath) ends with a PDF extension, implyingredirectPathis non-null. However, the multi-step validation makes this less obvious.📝 Optional: Add explicit null check for clarity
const redirectPath = sessionStorage.getItem(SEC_ADV_REDIRECT_PATH_KEY); + if (!redirectPath) { + return; + } - const redirectPathOnly = redirectPath?.split('?')[0] ?? ''; + const redirectPathOnly = redirectPath.split('?')[0]; if (!pathnameEndsWithPdf(redirectPathOnly)) { return; } const pathOnly = location.pathname; const onOAuthLanding = pathOnly === '/' || pathOnly === ''; if (onOAuthLanding) { - navigate(redirectPath!, { replace: true }); + navigate(redirectPath, { replace: true }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/security-advisory-patches-portal/webapp/src/app/AppHandler.tsx` around lines 33 - 51, The non-null assertion on navigate(redirectPath!, { replace: true }) is unclear; add an explicit early null check for redirectPath before using it: after retrieving redirectPath (const redirectPath = sessionStorage.getItem(SEC_ADV_REDIRECT_PATH_KEY)), verify redirectPath is truthy (e.g., if (!redirectPath) return;) so the subsequent redirectPathOnly/ pathnameEndsWithPdf check and the navigate call in the onOAuthLanding branch use a clearly validated non-null value; reference redirectPath, redirectPathOnly, pathnameEndsWithPdf, and navigate when making this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/security-advisory-patches-portal/backend/constants.bal`:
- Line 30: ALLOWED_PATH_PATTERN currently allows empty strings and control
whitespace; update the regex constant ALLOWED_PATH_PATTERN so it requires at
least one character (use + instead of *) and remove the shorthand \s, allowing
only a literal space character where needed; keep allowed characters (letters,
digits, hyphen, underscore, dot, literal space, percent-encoded hex and slash)
but ensure the percent-encoding token still accepts '%' followed by hex digits
and the overall pattern anchors remain intact.
In `@apps/security-advisory-patches-portal/backend/service.bal`:
- Around line 25-39: The /file endpoint (and the other open endpoints in the
block around lines 81-114) are currently unprotected; add server-side JWT
authentication by attaching a Ballerina auth provider to the service or by
adding an auth guard on the specific resource: update the service declaration
for "service / on new http:Listener(9090)" to require JWT validation (using the
Ballerina auth module, e.g., a JwtAuthenticator/Bearer authenticator configured
to validate Authorization: Bearer tokens) or change the resource "resource
function <name> /file" to accept an authenticated caller (inject the
authenticated subject in the resource signature) and return 401/403 when the
token is missing/invalid so direct API calls are rejected without a valid JWT.
- Around line 32-33: The CORS config currently lists write verbs (allowMethods:
["GET", "POST", "PUT", "PATCH", "DELETE", "OPTIONS"]) which is excessive for
this read-only API; update the allowMethods value in the CORS configuration (the
allowMethods setting) to only the HTTP verbs the service supports (e.g., "GET"
and "OPTIONS", and optionally "HEAD") and leave allowCredentials as needed, so
remove "POST", "PUT", "PATCH", and "DELETE" from the list to restrict exposure.
In `@apps/security-advisory-patches-portal/README.md`:
- Line 186: Update the Asgardeo redirect URL for consistency: replace the
occurrence of "http://patches.wso2.com:3000" (no trailing slash) with the
canonical "http://patches.wso2.com:3000/" so it matches the existing redirect
URI entry with a trailing slash and avoids OIDC exact-match login failures;
ensure any other references in the README that use the redirect string also use
the trailing-slash variant.
- Line 188: Reconcile the two README CORS mentions by explicitly stating whether
the origin is present by default in the backend/service.bal CORS list: open
backend/service.bal, inspect the CORS list/pattern that currently includes
"http://patches.wso2.com:3000", and then update the README sections (the
“default CORS” block that lists localhost origins and the line that references
backend/service.bal) to clearly say either “this origin is already included by
default in backend/service.bal” or “you must add this origin to
backend/service.bal”; ensure both places use the same wording and point readers
to backend/service.bal’s CORS list for how to add additional origins.
In `@apps/security-advisory-patches-portal/webapp/package.json`:
- Line 35: The package.json script "start:patches-host" uses a POSIX-only
environment prefix (HOST=patches.wso2.com ...) which breaks on Windows; update
the script to use cross-env (e.g., prefix with cross-env HOST=...) and do the
same for the other environment-prefixed scripts around lines 52-54 so they
become cross-platform, and add cross-env to devDependencies (or ensure it's
already installed) so npm/yarn can run the scripts on all OSes; target the
"start:patches-host" script name and the other env-prefixed scripts when making
the edits.
In `@apps/security-advisory-patches-portal/webapp/src/config/config.ts`:
- Around line 42-48: AsgardeoConfig is being populated with possibly empty
strings from window.config; add a validation step (e.g., near where
AsgardeoConfig is exported or during app startup) that verifies window.config
exists and that AUTH_SIGN_IN_REDIRECT_URL, AUTH_SIGN_OUT_REDIRECT_URL,
ASGARDEO_CLIENT_ID, and ASGARDEO_BASE_URL are non-empty; if any are missing,
throw a clear, fast-failing error (or call a bootstrap error handler) stating
which key(s) are missing so the Asgardeo SDK never receives empty values. Ensure
the check references AsgardeoConfig and the exact window.config property names
used in the file so maintainers can find and update the validation easily.
In `@apps/security-advisory-patches-portal/webapp/src/constants/constants.ts`:
- Around line 28-31: The function pathnameEndsWithPdf is not stripping query
strings (or fragments) so inputs like "/path/file.pdf?x=1" fail; update
pathnameEndsWithPdf to remove any query string and fragment before trimming and
splitting (e.g., strip everything from the first '?' or '#' out of pathname),
then proceed to compute trimmed, lastSegment and run /\.pdf$/i.test(lastSegment)
so the extension check ignores query parameters and fragments.
In `@apps/security-advisory-patches-portal/webapp/src/context/AuthContext.tsx`:
- Around line 71-81: The third else-if branch can call signIn() even after
isSignInInitiated was set, causing duplicate OAuth flows; remove that redundant
branch (the block that checks typeof window !== 'undefined' &&
pathnameEndsWithPdf(...) and calls
sessionStorage.setItem(SEC_ADV_REDIRECT_PATH_KEY, ...) then signIn()), or
replace it with a guard that checks isSignInInitiated before calling signIn();
ensure pathnameEndsWithPdf, SEC_ADV_SIGN_IN_INIT_KEY, SEC_ADV_REDIRECT_PATH_KEY,
and signIn() are used only in the initial branch that sets isSignInInitiated to
'true'.
- Around line 39-82: The effect uses window.location.pathname (calls to
pathnameEndsWithPdf) inside the useEffect tied to state.isAuthenticated but
omits pathname from deps, which trips exhaustive-deps; either (preferred)
refactor to read pathname from React Router by importing and using useLocation
and include location.pathname in the dependency array (update references in the
effect where window.location.pathname is used), or if this provider truly lives
outside Router and the omission is intentional, add an inline
eslint-disable-next-line comment above the useEffect with a short justification
referencing AppAuthProvider, state.isAuthenticated, and SEC_ADV_SIGN_IN_INIT_KEY
to explain why pathname is intentionally excluded.
- Around line 59-62: The AuthContext useEffect currently calls new
APIService(async () => { const token = await getIDToken(); return { idToken:
token || '' }; }); without retaining the instance, which can create multiple
APIService objects when state.isAuthenticated toggles; update the effect to
either store the created instance in a ref (e.g., apiServiceRef.current = new
APIService(...)) and reuse it across renders or replace the call with a proper
singleton accessor (e.g., APIService.getInstance(...)) so construction is
idempotent, and add cleanup/teardown in the effect return if APIService exposes
a dispose/unregister method to avoid leaks—look for APIService, getIDToken, the
useEffect that watches state.isAuthenticated, and add the ref/singleton and
optional cleanup there.
In `@apps/security-advisory-patches-portal/webapp/src/index.css`:
- Around line 16-17: The font-family declaration string in the CSS uses quoted
names that violate the font-family-name-quotes rule; edit the font-family list
in the declaration (the line starting with "font-family: 'Roboto',
-apple-system, BlinkMacSystemFont, 'Segoe UI', 'Oxygen', 'Ubuntu', 'Cantarell',
'Fira Sans', 'Droid Sans', 'Helvetica Neue',") by removing quotes from family
names that do not require them (e.g., Roboto, Oxygen, Ubuntu, Cantarell) while
keeping quotes for multi-word names that need them (e.g., "Segoe UI", "Fira
Sans", "Droid Sans", "Helvetica Neue"), so the list conforms to the lint rule.
In `@apps/security-advisory-patches-portal/webapp/src/index.tsx`:
- Around line 25-27: The current conditional using path.startsWith('/patches')
is too broad and matches routes like '/patches-legacy'; change the check to only
match exactly '/patches' or any sub-route under it by replacing the condition
with (path === '/patches' || path.startsWith('/patches/')). Keep the existing
behavior that calls sessionStorage.setItem(SEC_ADV_REDIRECT_PATH_KEY, path +
window.location.search) and sessionStorage.removeItem(SEC_ADV_SIGN_IN_INIT_KEY)
when the tightened condition is true; update the condition around those calls
(referencing path, SEC_ADV_REDIRECT_PATH_KEY, sessionStorage.setItem,
sessionStorage.removeItem) accordingly.
In `@apps/security-advisory-patches-portal/webapp/src/layout/Header.tsx`:
- Around line 108-131: The clickable user menu Box (the Box wrapping Avatar,
Typography username and ArrowDownIcon) lacks keyboard and ARIA support; update
that Box to include role="button", tabIndex={0}, aria-haspopup="true" and
aria-expanded={Boolean(anchorEl || isMenuOpen)} (use the existing menu
anchor/open state), and add an onKeyDown handler that calls handleMenuClick when
Enter (key === 'Enter' || keyCode === 13) or Space (key === ' ' || key ===
'Spacebar' || keyCode === 32) is pressed; keep handleMenuClick as the action to
toggle/open the menu so screenreader and keyboard users can operate the menu.
- Around line 91-100: The logo image currently uses onClick to navigate but
lacks keyboard accessibility; update the <img> in Header (the element with
onClick={() => navigate('/')} and the navigate function) to be keyboard
focusable and actionable by adding role="button", tabIndex={0}, and an onKeyDown
handler that calls navigate('/') when Enter or Space is pressed, or preferably
replace the <img> with a semantic <button> or React Router <Link> that wraps the
logo to provide native keyboard support and ARIA semantics.
In `@apps/security-advisory-patches-portal/webapp/src/utils/apiService.ts`:
- Around line 50-51: The retry config currently lists non-idempotent methods in
the http client config (httpMethodsToRetry: ['GET', 'POST', 'PATCH', 'PUT',
'DELETE']), which is unsafe for a download-only APIService; update the
httpMethodsToRetry to only include idempotent methods (e.g., ['GET', 'HEAD'] or
just ['GET']) in the HTTP client configuration used by APIService so retries on
401 won't replay mutating requests, leaving statusCodesToRetry as-is.
- Around line 34-45: The constructor is calling attach(APIService._instance)
unconditionally which can cause duplicate/interceptor stacking; change the
constructor to mirror getInstance() by only calling attach(...) and
APIService.updateRequestInterceptor() when APIService._initialized is false
(i.e., wrap attach and updateRequestInterceptor in a guard that checks
APIService._initialized or set _initialized before attaching), keeping
APIService._callback assignment but ensuring attach(APIService._instance) and
updateRequestInterceptor() run only once to prevent stacking retry interceptors.
In `@apps/security-advisory-patches-portal/webapp/src/utils/utils.ts`:
- Around line 30-39: decodePathSegment currently calls decodeURIComponent twice
with no error handling, which lets malformed percent-escapes throw URIError and
crash the route; wrap both decodeURIComponent calls inside a try/catch and on
error return a sentinel (e.g., null) instead of letting the exception propagate
so callers can detect invalid segments and render NotFound; update the caller
that maps over segments to check for a null return from decodePathSegment (or
handle a thrown specific Error) and short-circuit to the NotFound response as
appropriate.
---
Nitpick comments:
In `@apps/security-advisory-patches-portal/webapp/.gitignore`:
- Line 3: Remove the ignore entry for package-lock.json from .gitignore and
add/commit the actual package-lock.json file to the repo so dependency
resolution is reproducible; specifically delete the "package-lock.json" line in
the .gitignore and run git add/commit for package-lock.json (and push) so the
lockfile is tracked in version control.
In `@apps/security-advisory-patches-portal/webapp/config-overrides.js`:
- Around line 18-22: The dev-server currently sets config.devServer.allowedHosts
= 'all' which is too permissive; update the block that runs when env ===
'development' to replace the string 'all' with an explicit allowlist array
(e.g., ['localhost', '127.0.0.1', 'patches.wso2.com']) so
config.devServer.allowedHosts is an array of permitted hostnames; modify the
code near the config.devServer assignment to use that array (or read the list
from an environment variable and fallback to the explicit list) to narrow
allowed hosts.
In `@apps/security-advisory-patches-portal/webapp/src/app/AppHandler.tsx`:
- Around line 68-96: The duplicated centered loading UI in AppHandler (the
isLoading || !isAuthenticated block and the "Opening your link" block) should be
extracted into a reusable component (e.g., LoadingScreen) to remove repetition
and centralize styling; create a LoadingScreen React component that accepts a
message prop and renders the shared Box/CircularProgress/typography structure,
then replace the duplicated JSX in AppHandler with <LoadingScreen
message="..."/> (use the same sx styles and CircularProgress size) to keep
behavior identical.
- Around line 33-51: The non-null assertion on navigate(redirectPath!, {
replace: true }) is unclear; add an explicit early null check for redirectPath
before using it: after retrieving redirectPath (const redirectPath =
sessionStorage.getItem(SEC_ADV_REDIRECT_PATH_KEY)), verify redirectPath is
truthy (e.g., if (!redirectPath) return;) so the subsequent redirectPathOnly/
pathnameEndsWithPdf check and the navigate call in the onOAuthLanding branch use
a clearly validated non-null value; reference redirectPath, redirectPathOnly,
pathnameEndsWithPdf, and navigate when making this change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3879a894-753a-474d-bdde-20370023bbbb
⛔ Files ignored due to path filters (1)
apps/security-advisory-patches-portal/webapp/public/wso2-logo.svgis excluded by!**/*.svg
📒 Files selected for processing (39)
apps/security-advisory-patches-portal/README.mdapps/security-advisory-patches-portal/backend/Ballerina.tomlapps/security-advisory-patches-portal/backend/Config.toml.localapps/security-advisory-patches-portal/backend/Dependencies.tomlapps/security-advisory-patches-portal/backend/constants.balapps/security-advisory-patches-portal/backend/modules/file_storage/client.balapps/security-advisory-patches-portal/backend/modules/file_storage/constants.balapps/security-advisory-patches-portal/backend/modules/file_storage/file_storage.balapps/security-advisory-patches-portal/backend/modules/file_storage/types.balapps/security-advisory-patches-portal/backend/modules/file_storage/utils.balapps/security-advisory-patches-portal/backend/service.balapps/security-advisory-patches-portal/webapp/.eslintrc.jsapps/security-advisory-patches-portal/webapp/.gitignoreapps/security-advisory-patches-portal/webapp/config-overrides.jsapps/security-advisory-patches-portal/webapp/index.htmlapps/security-advisory-patches-portal/webapp/package.jsonapps/security-advisory-patches-portal/webapp/public/index.htmlapps/security-advisory-patches-portal/webapp/public/manifest.jsonapps/security-advisory-patches-portal/webapp/public/robots.txtapps/security-advisory-patches-portal/webapp/src/App.tsxapps/security-advisory-patches-portal/webapp/src/app/AppHandler.tsxapps/security-advisory-patches-portal/webapp/src/config/config.tsapps/security-advisory-patches-portal/webapp/src/constants/constants.tsapps/security-advisory-patches-portal/webapp/src/context/AuthContext.tsxapps/security-advisory-patches-portal/webapp/src/index.cssapps/security-advisory-patches-portal/webapp/src/index.tsxapps/security-advisory-patches-portal/webapp/src/layout/Header.tsxapps/security-advisory-patches-portal/webapp/src/slices/authSlice/auth.tsapps/security-advisory-patches-portal/webapp/src/slices/store.tsapps/security-advisory-patches-portal/webapp/src/theme.tsapps/security-advisory-patches-portal/webapp/src/types/images.d.tsapps/security-advisory-patches-portal/webapp/src/types/types.tsapps/security-advisory-patches-portal/webapp/src/utils/apiService.tsapps/security-advisory-patches-portal/webapp/src/utils/fileService.tsapps/security-advisory-patches-portal/webapp/src/utils/utils.tsapps/security-advisory-patches-portal/webapp/src/view/NotFound/NotFoundPage.tsxapps/security-advisory-patches-portal/webapp/src/view/PatchesPdf/PatchesPdfPage.tsxapps/security-advisory-patches-portal/webapp/src/view/RootLanding/RootLandingPage.tsxapps/security-advisory-patches-portal/webapp/tsconfig.json
…te-aligned OIDC Add authorization module (JwtInterceptor, checkPermissions), interceptors on the HTTP service, and ID token on x-jwt-assertion with openid/email/groups scopes. Document gateway CORS instead of embedding CORS in the Ballerina listener. Leave webapps/ templates untracked.
Contributor
Author
|
@coderabbitai can you do a fresh review on the PR because I've integrated the JWT authentication for the backend endpoint. |
Contributor
|
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (2)
apps/security-advisory-patches-portal/README.md (1)
199-199:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix redirect URL trailing slash for consistency.
Line 197 shows the config with a trailing slash (
http://patches.wso2.com:3000/), but Line 199 instructs adding the redirect URL without a trailing slash (http://patches.wso2.com:3000). OIDC providers perform exact string matching on redirect URIs, so this inconsistency can cause authentication failures.Use the same format consistently—prefer the version with trailing slash everywhere to match the canonical redirect URI pattern.
📝 Suggested fix
- - In Asgardeo, add **`http://patches.wso2.com:3000`** as an allowed redirect URL. + - In Asgardeo, add **`http://patches.wso2.com:3000/`** as an allowed redirect URL.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/security-advisory-patches-portal/README.md` at line 199, The README contains inconsistent redirect URIs: the config lists "http://patches.wso2.com:3000/" (with trailing slash) but the instruction line uses "http://patches.wso2.com:3000" (without trailing slash); update the instruction text to use the trailing-slash form to match the canonical redirect URI used in the config so OIDC exact-match validation succeeds—replace the non-slash URL in the README instruction with "http://patches.wso2.com:3000/" wherever mentioned to ensure consistency.apps/security-advisory-patches-portal/backend/constants.bal (1)
30-30:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHarden
ALLOWED_PATH_PATTERNon Line 30 to block empty/control-whitespace paths and malformed%escapes.The current pattern still allows empty values, control whitespace via
\s, and standalone%characters. For a security-sensitive file path input, this is too permissive.Suggested patch
-const string ALLOWED_PATH_PATTERN = "^[a-zA-Z0-9\\-_\\.\\s%0-9A-Fa-f/]*$"; +const string ALLOWED_PATH_PATTERN = "^(?:[a-zA-Z0-9_. /_-]|%[0-9A-Fa-f]{2})+$";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/security-advisory-patches-portal/backend/constants.bal` at line 30, ALLOWED_PATH_PATTERN is too permissive: it allows empty strings, control whitespace via \s, and lone '%' characters; update the constant ALLOWED_PATH_PATTERN to require at least one character, disallow control whitespace (permit only the literal space if desired), and enforce that any '%' appears only as a proper percent-encoding (% followed by exactly two hex digits). Locate the ALLOWED_PATH_PATTERN constant and replace the pattern with one that (1) uses a start/end anchor and a positive-length requirement so empty strings fail, (2) omits \s and only allows the literal space character if needed, and (3) validates percent-escapes with %[0-9A-Fa-f]{2} so stray '%' are rejected. Ensure the new regex is used everywhere the constant is referenced.
🧹 Nitpick comments (1)
apps/security-advisory-patches-portal/webapp/.gitignore (1)
3-3: ⚡ Quick winConsider committing
package-lock.jsonfor reproducible builds.The
package-lock.jsonfile is typically committed to version control to ensure that all developers and CI/CD environments install identical dependency versions. Ignoring it can lead to version drift, non-reproducible builds, and potential inconsistencies across environments.📦 Recommended change
node_modules/ public/config.js -package-lock.json build/🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/security-advisory-patches-portal/webapp/.gitignore` at line 3, Remove package-lock.json from the .gitignore so the lockfile is tracked and commit the generated package-lock.json to the repo; locate the .gitignore entry for "package-lock.json" and delete that line, then run npm install (to generate package-lock.json if missing), git add package-lock.json and commit so npm installs are reproducible across dev and CI.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@apps/security-advisory-patches-portal/backend/modules/authorization/authorization.bal`:
- Around line 37-60: The handlers that currently return http:InternalServerError
for JWT/header issues should return client auth errors: when
JWT_ASSERTION_HEADER is missing (idToken is error) return an http:Unauthorized
(401) with an appropriate message; when jwt:decode(idToken) yields jwt:Error
return http:Unauthorized (401); when cloning to CustomJwtPayload fails (userInfo
is error) return http:BadRequest (400) since the token payload is malformed.
Update the return expressions that currently construct http:InternalServerError
in these checks and keep the same log:printError calls and messages, only
changing the response type and status to http:Unauthorized or http:BadRequest as
described.
- Line 69: The current log statement in the permission-denied path uses
log:printError with raw PII (userInfo.email and userInfo.groups.toBalString());
change it to avoid logging sensitive data by removing or redacting the email and
full group list and instead emit a non-PII message (e.g., "permission denied for
user [id:<redacted>]" or log a hashed/partial email or group count) and include
only necessary context like a request/correlation id; update the log call (the
log:printError invocation referencing userInfo.email and
userInfo.groups.toBalString()) to use the redacted/hashed identifier or minimal
metadata and keep the error-level log message descriptive but PII-free.
In
`@apps/security-advisory-patches-portal/backend/modules/file_storage/utils.bal`:
- Around line 23-33: validatePath currently only checks length; update
validatePath to early-reject malformed or traversal-like inputs and return
error(ERR_MSG_INVALID_PATH) for invalid cases. Specifically, after the
empty-string and MAX_PATH_LENGTH checks, ensure the path: does not start with
'/' or contain backslashes '\' or null chars, does not contain empty segments
(no '//' occurrences), and that none of its path segments are '.' or '..' (split
on '/'); if any of these conditions fail return error(ERR_MSG_INVALID_PATH).
Keep existing length check against MAX_PATH_LENGTH and preserve the function
signature and isolated behavior.
In `@apps/security-advisory-patches-portal/backend/service.bal`:
- Line 143: The Content-Disposition header is vulnerable to CRLF injection
because ALLOWED_PATH_PATTERN currently allows whitespace (\s) so an URL-decoded
path can contain CR/LF which then flows into
response.setHeader("Content-Disposition", string `inline;
filename="${fileName}"`); — fix by tightening ALLOWED_PATH_PATTERN to disallow
control and whitespace characters (remove \s and only allow [A-Za-z0-9\-_.%/]+
or the suggested pattern) and/or additionally sanitize/escape the fileName
before calling response.setHeader (reject or percent-encode CR/LF/control chars
and strip newlines) so fileName can never inject headers; update the pattern
constant and add validation logic where fileName is derived and before the
response.setHeader call.
In `@apps/security-advisory-patches-portal/README.md`:
- Line 102: README.md and in-code comments reference non-existent
webapps/backend-template and webapps/webapp-template, producing broken links;
remove or correct these references by either linking to the correct existing
templates or deleting the broken links and explanatory text. Update README.md
(the line referencing `Config.toml`), and fix comments in
webapp/src/utils/apiService.ts, webapp/src/utils/fileService.ts, and
webapp/src/config/config.ts: replace references to
`webapps/backend-template`/`webapps/webapp-template` with the correct repo paths
or a neutral description of the expected Config.toml layout (Azure file share +
authorization), and ensure any relative links point to valid targets or are
removed so they no longer produce 404s.
In `@apps/security-advisory-patches-portal/webapp/config-overrides.js`:
- Around line 16-23: The use of allowedHosts: 'all' in the override function
disables host-header validation on config.devServer and risks DNS rebinding;
replace it with a safe allowlist (for example use 'auto' or an explicit array
like ['patches.wso2.com','localhost'] or ['.wso2.com'] for subdomains) within
the module.exports override function so config.devServer.allowedHosts is set to
the restricted value instead of 'all'. Ensure you update the override function
that mutates config.devServer and keep the rest of the spread merge
(...config.devServer) behavior intact.
In `@apps/security-advisory-patches-portal/webapp/public/manifest.json`:
- Around line 4-10: The manifest's SVG icon entry uses an incorrect pixel size
string; for the "icons" entry where "src": "wso2-logo.svg" and "type":
"image/svg+xml", change the "sizes" value from "512x512" to "any" to comply with
the Web App Manifest spec (and scan other icon entries to ensure any SVGs
likewise use "any" while raster images keep explicit pixel sizes).
In `@apps/security-advisory-patches-portal/webapp/src/config/config.ts`:
- Around line 51-59: serviceBaseUrl may include a trailing slash from
window.config.BACKEND_BASE_URL which leads to double slashes in downloadFileUrl
(e.g., //file); before building AppConfig.downloadFileUrl trim any trailing
slashes from serviceBaseUrl (normalize window.config?.BACKEND_BASE_URL by
removing trailing '/' characters) and then use the normalized value when
constructing downloadFileUrl so `${serviceBaseUrl}/file` never produces
`//file`; update the code that sets serviceBaseUrl (referencing serviceBaseUrl,
AppConfig, downloadFileUrl) to perform this normalization.
---
Duplicate comments:
In `@apps/security-advisory-patches-portal/backend/constants.bal`:
- Line 30: ALLOWED_PATH_PATTERN is too permissive: it allows empty strings,
control whitespace via \s, and lone '%' characters; update the constant
ALLOWED_PATH_PATTERN to require at least one character, disallow control
whitespace (permit only the literal space if desired), and enforce that any '%'
appears only as a proper percent-encoding (% followed by exactly two hex
digits). Locate the ALLOWED_PATH_PATTERN constant and replace the pattern with
one that (1) uses a start/end anchor and a positive-length requirement so empty
strings fail, (2) omits \s and only allows the literal space character if
needed, and (3) validates percent-escapes with %[0-9A-Fa-f]{2} so stray '%' are
rejected. Ensure the new regex is used everywhere the constant is referenced.
In `@apps/security-advisory-patches-portal/README.md`:
- Line 199: The README contains inconsistent redirect URIs: the config lists
"http://patches.wso2.com:3000/" (with trailing slash) but the instruction line
uses "http://patches.wso2.com:3000" (without trailing slash); update the
instruction text to use the trailing-slash form to match the canonical redirect
URI used in the config so OIDC exact-match validation succeeds—replace the
non-slash URL in the README instruction with "http://patches.wso2.com:3000/"
wherever mentioned to ensure consistency.
---
Nitpick comments:
In `@apps/security-advisory-patches-portal/webapp/.gitignore`:
- Line 3: Remove package-lock.json from the .gitignore so the lockfile is
tracked and commit the generated package-lock.json to the repo; locate the
.gitignore entry for "package-lock.json" and delete that line, then run npm
install (to generate package-lock.json if missing), git add package-lock.json
and commit so npm installs are reproducible across dev and CI.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a6b1b13f-ff5c-4293-9e07-add4fa9f3a9c
⛔ Files ignored due to path filters (1)
apps/security-advisory-patches-portal/webapp/public/wso2-logo.svgis excluded by!**/*.svg
📒 Files selected for processing (43)
apps/security-advisory-patches-portal/README.mdapps/security-advisory-patches-portal/backend/Ballerina.tomlapps/security-advisory-patches-portal/backend/Config.toml.localapps/security-advisory-patches-portal/backend/Dependencies.tomlapps/security-advisory-patches-portal/backend/constants.balapps/security-advisory-patches-portal/backend/modules/authorization/authorization.balapps/security-advisory-patches-portal/backend/modules/authorization/constants.balapps/security-advisory-patches-portal/backend/modules/authorization/types.balapps/security-advisory-patches-portal/backend/modules/authorization/utils.balapps/security-advisory-patches-portal/backend/modules/file_storage/client.balapps/security-advisory-patches-portal/backend/modules/file_storage/constants.balapps/security-advisory-patches-portal/backend/modules/file_storage/file_storage.balapps/security-advisory-patches-portal/backend/modules/file_storage/types.balapps/security-advisory-patches-portal/backend/modules/file_storage/utils.balapps/security-advisory-patches-portal/backend/service.balapps/security-advisory-patches-portal/webapp/.eslintrc.jsapps/security-advisory-patches-portal/webapp/.gitignoreapps/security-advisory-patches-portal/webapp/config-overrides.jsapps/security-advisory-patches-portal/webapp/index.htmlapps/security-advisory-patches-portal/webapp/package.jsonapps/security-advisory-patches-portal/webapp/public/index.htmlapps/security-advisory-patches-portal/webapp/public/manifest.jsonapps/security-advisory-patches-portal/webapp/public/robots.txtapps/security-advisory-patches-portal/webapp/src/App.tsxapps/security-advisory-patches-portal/webapp/src/app/AppHandler.tsxapps/security-advisory-patches-portal/webapp/src/config/config.tsapps/security-advisory-patches-portal/webapp/src/constants/constants.tsapps/security-advisory-patches-portal/webapp/src/context/AuthContext.tsxapps/security-advisory-patches-portal/webapp/src/index.cssapps/security-advisory-patches-portal/webapp/src/index.tsxapps/security-advisory-patches-portal/webapp/src/layout/Header.tsxapps/security-advisory-patches-portal/webapp/src/slices/authSlice/auth.tsapps/security-advisory-patches-portal/webapp/src/slices/store.tsapps/security-advisory-patches-portal/webapp/src/theme.tsapps/security-advisory-patches-portal/webapp/src/types/images.d.tsapps/security-advisory-patches-portal/webapp/src/types/types.tsapps/security-advisory-patches-portal/webapp/src/utils/apiService.tsapps/security-advisory-patches-portal/webapp/src/utils/fileService.tsapps/security-advisory-patches-portal/webapp/src/utils/utils.tsapps/security-advisory-patches-portal/webapp/src/view/NotFound/NotFoundPage.tsxapps/security-advisory-patches-portal/webapp/src/view/PatchesPdf/PatchesPdfPage.tsxapps/security-advisory-patches-portal/webapp/src/view/RootLanding/RootLandingPage.tsxapps/security-advisory-patches-portal/webapp/tsconfig.json
Clarified roles and updated CORS configuration details in README.
…:3000 Drop webpack allowedHosts override and start:patches-host; document Asgardeo redirects for http://localhost:3000 only; align root landing copy with that.
…ntent-Disposition Harden ALLOWED_PATH_PATTERN and validatePath before Azure calls; set Content-Disposition via RFC 5987 filename* with url-encoded basename. No CORS on the listener (gateway/proxy for cross-origin).
Stop logging email and full groups on 403; keep a generic ops message instead.
Contributor
Author
|
@coderabbitai I've resolved your comments. Can you check if everything is good? |
Contributor
|
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (8)
apps/security-advisory-patches-portal/webapp/src/index.tsx (1)
25-27:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTighten
/patchesroute matching before saving redirect state.At Line 25,
startsWith('/patches')also matches unrelated paths like/patches-legacy. Restrict this to exact/patchesor/patches/...only.🔧 Suggested change
- if (path.startsWith('/patches')) { + if (path === '/patches' || path.startsWith('/patches/')) { sessionStorage.setItem(SEC_ADV_REDIRECT_PATH_KEY, path + window.location.search); sessionStorage.removeItem(SEC_ADV_SIGN_IN_INIT_KEY); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/security-advisory-patches-portal/webapp/src/index.tsx` around lines 25 - 27, Change the loose route check that uses path.startsWith('/patches') to a stricter match so only the exact "/patches" or any subpath "/patches/..." saves redirect state; update the conditional around where sessionStorage.setItem(SEC_ADV_REDIRECT_PATH_KEY, path + window.location.search) and sessionStorage.removeItem(SEC_ADV_SIGN_IN_INIT_KEY) are called to verify either path === '/patches' or path.startsWith('/patches/') (using the existing path variable) before writing to sessionStorage so routes like "/patches-legacy" are ignored.apps/security-advisory-patches-portal/webapp/src/index.css (1)
16-17:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix
font-familyquotes to satisfy the configured stylelint rule.At Line 16 and Line 17, single-word families should be unquoted (
Roboto,Oxygen,Ubuntu,Cantarell) to passfont-family-name-quotes.🔧 Suggested change
- font-family: 'Roboto', -apple-system, BlinkMacSystemFont, 'Segoe UI', 'Oxygen', - 'Ubuntu', 'Cantarell', 'Fira Sans', 'Droid Sans', 'Helvetica Neue', + font-family: Roboto, -apple-system, BlinkMacSystemFont, 'Segoe UI', Oxygen, + Ubuntu, Cantarell, 'Fira Sans', 'Droid Sans', 'Helvetica Neue', sans-serif;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/security-advisory-patches-portal/webapp/src/index.css` around lines 16 - 17, The font-family declaration contains single-word family names wrapped in single quotes which violates the font-family-name-quotes rule; edit the font-family list in index.css (the font-family declaration shown) to remove quotes from single-word families such as Roboto, Oxygen, Ubuntu, Cantarell while preserving quotes around multi-word family names like 'Segoe UI', 'Fira Sans', 'Droid Sans', and 'Helvetica Neue' so the rule passes.apps/security-advisory-patches-portal/webapp/src/utils/apiService.ts (1)
42-47:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
attach()to avoid duplicate retry interceptors.Line 42 still attaches retry-axios unconditionally in the constructor. This can stack interceptors if the class is instantiated more than once.
🔧 Minimal fix
- attach(APIService._instance); + if (!APIService._initialized) { + attach(APIService._instance); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/security-advisory-patches-portal/webapp/src/utils/apiService.ts` around lines 42 - 47, The call to attach() is unconditional and can add duplicate retry-axios interceptors when the class is instantiated multiple times; guard it with a static flag so attach() runs only once (e.g., add APIService._retryAttached boolean, check it before calling attach(), and set it true immediately after attach()), and ensure any other code paths (such as APIService.updateRequestInterceptor() or the constructor that sets APIService._instance, APIService._callback and APIService._initialized) do not call attach() again without that guard.apps/security-advisory-patches-portal/webapp/src/layout/Header.tsx (2)
91-100:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake the logo navigation keyboard-accessible.
On Lines 91-100, the logo is mouse-clickable only. Keyboard users cannot activate it.
♿ Suggested fix
- <img - src="/wso2-logo.svg" - alt="WSO2 Logo" - style={{ - height: 32, - marginRight: 16, - cursor: 'pointer', - }} - onClick={() => navigate('/')} - /> + <Box + component="button" + onClick={() => navigate('/')} + sx={{ + border: 0, + background: 'transparent', + padding: 0, + cursor: 'pointer', + marginRight: 2, + }} + aria-label="Go to home" + > + <img + src="/wso2-logo.svg" + alt="WSO2 Logo" + style={{ height: 32 }} + /> + </Box>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/security-advisory-patches-portal/webapp/src/layout/Header.tsx` around lines 91 - 100, The logo <img> is only mouse-clickable via its onClick handler (onClick={() => navigate('/')}) so keyboard users can't activate it; make it keyboard-accessible by adding a tabindex (tabIndex={0}), a semantic accessible name (e.g. aria-label="Home" or aria-hidden=false as appropriate), a role="button" and an onKeyDown handler that calls the same navigate('/') when Enter or Space is pressed (mirror the existing onClick behavior), and ensure focus styling is preserved so keyboard users can see focus. Reference the <img> element with its onClick and navigate('/') to locate and update the code.
108-131:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd keyboard + ARIA semantics for the user menu trigger.
On Lines 108-131, the clickable container is not keyboard-operable and does not expose menu semantics (
aria-haspopup/aria-expanded).♿ Suggested fix
<Box sx={{ display: 'flex', alignItems: 'center', cursor: 'pointer', '&:hover': { opacity: 0.8, }, }} onClick={handleMenuClick} + onKeyDown={(e: React.KeyboardEvent<HTMLElement>) => { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault(); + handleMenuClick(e as unknown as React.MouseEvent<HTMLElement>); + } + }} + role="button" + tabIndex={0} + aria-haspopup="menu" + aria-expanded={menuOpen} >🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/security-advisory-patches-portal/webapp/src/layout/Header.tsx` around lines 108 - 131, The user menu trigger Box is not keyboard accessible or exposing ARIA menu semantics; update the Box that contains Avatar/username/ArrowDownIcon (the element using onClick={handleMenuClick}) to be focusable and announce the menu: add role="button" and tabIndex={0}, add an onKeyDown handler that triggers handleMenuClick when Enter or Space is pressed, and add aria-haspopup="true" plus aria-expanded tied to the menu open state (e.g., use an isMenuOpen or menuOpen boolean used by the menu component) so screen readers know the control opens a menu.apps/security-advisory-patches-portal/webapp/src/context/AuthContext.tsx (2)
78-81:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRemove the second
signIn()trigger while sign-in is already initiated.On Lines 78-81, calling
signIn()again whenSEC_ADV_SIGN_IN_INIT_KEYis alreadytruecan create duplicate OAuth redirects/races.✂️ Suggested fix
- } else if (typeof window !== 'undefined' && pathnameEndsWithPdf(window.location.pathname)) { - sessionStorage.setItem(SEC_ADV_REDIRECT_PATH_KEY, window.location.pathname + window.location.search); - signIn(); - } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/security-advisory-patches-portal/webapp/src/context/AuthContext.tsx` around lines 78 - 81, The branch that handles PDF paths calls signIn() unconditionally, causing duplicate OAuth redirects when a sign-in is already initiated; in the AuthContext.tsx block that uses pathnameEndsWithPdf and SEC_ADV_REDIRECT_PATH_KEY, stop triggering signIn() when SEC_ADV_SIGN_IN_INIT_KEY indicates sign-in is in progress by checking sessionStorage.getItem(SEC_ADV_SIGN_IN_INIT_KEY) (or equivalent) before calling signIn(); always set the redirect path (SEC_ADV_REDIRECT_PATH_KEY) but only invoke signIn() if the sign-in-init flag is not already set to true.
59-62:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid re-instantiating
APIServiceduring each auth sync.On Line 59, constructing
new APIService(...)inside the effect can run multiple times across auth state transitions and cause unpredictable singleton/interceptor behavior.♻️ Suggested fix
-import React, { createContext, useContext, useEffect } from 'react'; +import React, { createContext, useContext, useEffect, useRef } from 'react'; ... + const apiServiceInitialized = useRef(false); ... - new APIService(async () => { - const token = await getIDToken(); - return { idToken: token || '' }; - }); + if (!apiServiceInitialized.current) { + new APIService(async () => { + const token = await getIDToken(); + return { idToken: token || '' }; + }); + apiServiceInitialized.current = true; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/security-advisory-patches-portal/webapp/src/context/AuthContext.tsx` around lines 59 - 62, The code currently creates a new APIService inside the auth sync effect (new APIService(async () => { const token = await getIDToken(); ... })), which can re-instantiate on each auth transition and break singleton/interceptor assumptions; fix it by creating the APIService once and reusing it (e.g. instantiate APIService outside the effect or store it in a React ref/context) and update its auth token provider or headers on auth changes instead of reconstructing the service; ensure references to APIService, getIDToken, and the auth sync effect are updated so only one APIService instance exists for the app lifetime.apps/security-advisory-patches-portal/webapp/src/utils/utils.ts (1)
30-39:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
decodeURIComponentto prevent malformed URL crashes.On Lines 30-39, malformed percent-encoding throws and propagates through Line 70 (
map), which can break route handling instead of returningnull.🛡️ Suggested fix
function decodePathSegment(segment: string): string { - if (segment.includes('%20')) { - return decodeURIComponent(segment); - } - const raw = decodeURIComponent(segment); + let raw: string; + try { + raw = decodeURIComponent(segment); + } catch { + return segment; + } + if (segment.includes('%20')) { + return raw; + } if (!raw.includes('--')) { return raw; } return unescapeDashSegment(raw); }Does JavaScript decodeURIComponent throw URIError on malformed percent-escape sequences (e.g., "%E0%A4%A")?Also applies to: 70-70
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/security-advisory-patches-portal/webapp/src/utils/utils.ts` around lines 30 - 39, The decodePathSegment function can throw from decodeURIComponent for malformed percent-encodings; wrap decodeURIComponent calls in a try/catch inside decodePathSegment (both the initial check and the later decode) and return null (or a sentinel) on error so callers (e.g., the code that maps over segments around the map usage) can skip/handle invalid segments; specifically catch URIError, avoid rethrowing, and ensure callers check for null before further processing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@apps/security-advisory-patches-portal/backend/modules/authorization/authorization.bal`:
- Around line 48-53: The code currently uses jwt:decode() to parse idToken and
then trusts the groups claim for RBAC (see usage of groups at the authorization
check), which is insecure; replace jwt:decode() with jwt:validate() using a
properly populated jwt:ValidatorConfig (issuer, audience, jwks_uri/JWKs) as done
in the customer-portal module, then branch on the validation result (handle
success vs jwt:Error) and extract the groups claim from the validated payload
only after successful validation; ensure validation errors are logged and return
the same InternalServerError flow when validation fails.
In `@apps/security-advisory-patches-portal/backend/service.bal`:
- Around line 119-124: Replace the inline regex path check with the centralized
validator by calling file_storage:validatePath(filePath) instead of using
filePath.matches(re `${ALLOWED_PATH_PATTERN}`); if validatePath indicates
invalid, log the error (include filePath) and return the same <http:BadRequest>{
body: {message: ERR_MSG_INVALID_PATH} } response. Update the block around
filePath (the same scope that currently uses ALLOWED_PATH_PATTERN) to use
file_storage:validatePath so all traversal/format rules are enforced in one
place and avoid duplicating validation logic.
In `@apps/security-advisory-patches-portal/webapp/.gitignore`:
- Line 3: Remove the package-lock.json entry from the .gitignore so the lockfile
is checked into source control (stop ignoring the symbol "package-lock.json");
then add and commit the package-lock.json file to the repo so installs are
deterministic and auditable across environments, ensuring future updates update
the lockfile and are reviewed.
In `@apps/security-advisory-patches-portal/webapp/index.html`:
- Around line 2-7: Replace the restrictive proprietary header at the top of
index.html with the repository's standard Apache-style license header: include
the SPDX identifier (e.g., "SPDX-License-Identifier: Apache-2.0"), the correct
copyright owner (WSO2 LLC) and year(s), and the short Apache 2.0 notice text
used by adjacent files so the file's licensing matches the rest of the repo;
ensure the new header replaces the existing block starting at the top of
index.html exactly and follows the same comment style used in that file.
In `@apps/security-advisory-patches-portal/webapp/src/context/AuthContext.tsx`:
- Around line 66-70: The catch block for auth initialization currently logs the
error and dispatches setAuthenticated(false) and setLoading(false) but does not
reset SEC_ADV_SIGN_IN_INIT_KEY; update the catch handler (the ).catch((error) =>
{ ... }) to clear or reset the sign-in-init flag (e.g.,
localStorage.removeItem(SEC_ADV_SIGN_IN_INIT_KEY) or
localStorage.setItem(SEC_ADV_SIGN_IN_INIT_KEY, 'false')) so future retries
aren’t suppressed—perform this immediately before or after dispatching
setLoading(false) to ensure the app can attempt sign-in again.
---
Duplicate comments:
In `@apps/security-advisory-patches-portal/webapp/src/context/AuthContext.tsx`:
- Around line 78-81: The branch that handles PDF paths calls signIn()
unconditionally, causing duplicate OAuth redirects when a sign-in is already
initiated; in the AuthContext.tsx block that uses pathnameEndsWithPdf and
SEC_ADV_REDIRECT_PATH_KEY, stop triggering signIn() when
SEC_ADV_SIGN_IN_INIT_KEY indicates sign-in is in progress by checking
sessionStorage.getItem(SEC_ADV_SIGN_IN_INIT_KEY) (or equivalent) before calling
signIn(); always set the redirect path (SEC_ADV_REDIRECT_PATH_KEY) but only
invoke signIn() if the sign-in-init flag is not already set to true.
- Around line 59-62: The code currently creates a new APIService inside the auth
sync effect (new APIService(async () => { const token = await getIDToken(); ...
})), which can re-instantiate on each auth transition and break
singleton/interceptor assumptions; fix it by creating the APIService once and
reusing it (e.g. instantiate APIService outside the effect or store it in a
React ref/context) and update its auth token provider or headers on auth changes
instead of reconstructing the service; ensure references to APIService,
getIDToken, and the auth sync effect are updated so only one APIService instance
exists for the app lifetime.
In `@apps/security-advisory-patches-portal/webapp/src/index.css`:
- Around line 16-17: The font-family declaration contains single-word family
names wrapped in single quotes which violates the font-family-name-quotes rule;
edit the font-family list in index.css (the font-family declaration shown) to
remove quotes from single-word families such as Roboto, Oxygen, Ubuntu,
Cantarell while preserving quotes around multi-word family names like 'Segoe
UI', 'Fira Sans', 'Droid Sans', and 'Helvetica Neue' so the rule passes.
In `@apps/security-advisory-patches-portal/webapp/src/index.tsx`:
- Around line 25-27: Change the loose route check that uses
path.startsWith('/patches') to a stricter match so only the exact "/patches" or
any subpath "/patches/..." saves redirect state; update the conditional around
where sessionStorage.setItem(SEC_ADV_REDIRECT_PATH_KEY, path +
window.location.search) and sessionStorage.removeItem(SEC_ADV_SIGN_IN_INIT_KEY)
are called to verify either path === '/patches' or path.startsWith('/patches/')
(using the existing path variable) before writing to sessionStorage so routes
like "/patches-legacy" are ignored.
In `@apps/security-advisory-patches-portal/webapp/src/layout/Header.tsx`:
- Around line 91-100: The logo <img> is only mouse-clickable via its onClick
handler (onClick={() => navigate('/')}) so keyboard users can't activate it;
make it keyboard-accessible by adding a tabindex (tabIndex={0}), a semantic
accessible name (e.g. aria-label="Home" or aria-hidden=false as appropriate), a
role="button" and an onKeyDown handler that calls the same navigate('/') when
Enter or Space is pressed (mirror the existing onClick behavior), and ensure
focus styling is preserved so keyboard users can see focus. Reference the <img>
element with its onClick and navigate('/') to locate and update the code.
- Around line 108-131: The user menu trigger Box is not keyboard accessible or
exposing ARIA menu semantics; update the Box that contains
Avatar/username/ArrowDownIcon (the element using onClick={handleMenuClick}) to
be focusable and announce the menu: add role="button" and tabIndex={0}, add an
onKeyDown handler that triggers handleMenuClick when Enter or Space is pressed,
and add aria-haspopup="true" plus aria-expanded tied to the menu open state
(e.g., use an isMenuOpen or menuOpen boolean used by the menu component) so
screen readers know the control opens a menu.
In `@apps/security-advisory-patches-portal/webapp/src/utils/apiService.ts`:
- Around line 42-47: The call to attach() is unconditional and can add duplicate
retry-axios interceptors when the class is instantiated multiple times; guard it
with a static flag so attach() runs only once (e.g., add
APIService._retryAttached boolean, check it before calling attach(), and set it
true immediately after attach()), and ensure any other code paths (such as
APIService.updateRequestInterceptor() or the constructor that sets
APIService._instance, APIService._callback and APIService._initialized) do not
call attach() again without that guard.
In `@apps/security-advisory-patches-portal/webapp/src/utils/utils.ts`:
- Around line 30-39: The decodePathSegment function can throw from
decodeURIComponent for malformed percent-encodings; wrap decodeURIComponent
calls in a try/catch inside decodePathSegment (both the initial check and the
later decode) and return null (or a sentinel) on error so callers (e.g., the
code that maps over segments around the map usage) can skip/handle invalid
segments; specifically catch URIError, avoid rethrowing, and ensure callers
check for null before further processing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ba401af4-5ea6-448e-a664-cdd206e798eb
⛔ Files ignored due to path filters (1)
apps/security-advisory-patches-portal/webapp/public/wso2-logo.svgis excluded by!**/*.svg
📒 Files selected for processing (43)
apps/security-advisory-patches-portal/README.mdapps/security-advisory-patches-portal/backend/Ballerina.tomlapps/security-advisory-patches-portal/backend/Config.toml.localapps/security-advisory-patches-portal/backend/Dependencies.tomlapps/security-advisory-patches-portal/backend/constants.balapps/security-advisory-patches-portal/backend/modules/authorization/authorization.balapps/security-advisory-patches-portal/backend/modules/authorization/constants.balapps/security-advisory-patches-portal/backend/modules/authorization/types.balapps/security-advisory-patches-portal/backend/modules/authorization/utils.balapps/security-advisory-patches-portal/backend/modules/file_storage/client.balapps/security-advisory-patches-portal/backend/modules/file_storage/constants.balapps/security-advisory-patches-portal/backend/modules/file_storage/file_storage.balapps/security-advisory-patches-portal/backend/modules/file_storage/types.balapps/security-advisory-patches-portal/backend/modules/file_storage/utils.balapps/security-advisory-patches-portal/backend/service.balapps/security-advisory-patches-portal/webapp/.eslintrc.jsapps/security-advisory-patches-portal/webapp/.gitignoreapps/security-advisory-patches-portal/webapp/config-overrides.jsapps/security-advisory-patches-portal/webapp/index.htmlapps/security-advisory-patches-portal/webapp/package.jsonapps/security-advisory-patches-portal/webapp/public/index.htmlapps/security-advisory-patches-portal/webapp/public/manifest.jsonapps/security-advisory-patches-portal/webapp/public/robots.txtapps/security-advisory-patches-portal/webapp/src/App.tsxapps/security-advisory-patches-portal/webapp/src/app/AppHandler.tsxapps/security-advisory-patches-portal/webapp/src/config/config.tsapps/security-advisory-patches-portal/webapp/src/constants/constants.tsapps/security-advisory-patches-portal/webapp/src/context/AuthContext.tsxapps/security-advisory-patches-portal/webapp/src/index.cssapps/security-advisory-patches-portal/webapp/src/index.tsxapps/security-advisory-patches-portal/webapp/src/layout/Header.tsxapps/security-advisory-patches-portal/webapp/src/slices/authSlice/auth.tsapps/security-advisory-patches-portal/webapp/src/slices/store.tsapps/security-advisory-patches-portal/webapp/src/theme.tsapps/security-advisory-patches-portal/webapp/src/types/images.d.tsapps/security-advisory-patches-portal/webapp/src/types/types.tsapps/security-advisory-patches-portal/webapp/src/utils/apiService.tsapps/security-advisory-patches-portal/webapp/src/utils/fileService.tsapps/security-advisory-patches-portal/webapp/src/utils/utils.tsapps/security-advisory-patches-portal/webapp/src/view/NotFound/NotFoundPage.tsxapps/security-advisory-patches-portal/webapp/src/view/PatchesPdf/PatchesPdfPage.tsxapps/security-advisory-patches-portal/webapp/src/view/RootLanding/RootLandingPage.tsxapps/security-advisory-patches-portal/webapp/tsconfig.json
…e name configuration.
…flow Reuse file_storage validatePath for GET /file instead of a separate regex. Reset SEC_ADV_SIGN_IN_INIT_KEY when getBasicUserInfo fails so sign-in can retry. README: align path and troubleshooting notes with segment validation and JWT decode behavior.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/security-advisory-patches-portal/README.md`:
- Around line 182-183: Update the troubleshooting table to match current backend
behavior: replace the two 500 entries with a single 400 entry that references
the actual backend fallback message "User information header not found!" (as
emitted from backend/service.bal), and keep guidance about the SPA not sending
the x-jwt-assertion header (see apiService.ts) and about ensuring the ID token
includes email and groups (use scopes openid,email,groups in
webapp/src/config/config.ts and enable these attributes in Asgardeo and assign
the user to the advisoryPatchesReaderRole group). Ensure the table text and
status code are consistent with the backend's runtime response and error string.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f89c3810-6345-4a53-9a30-4e59e3d735f5
📒 Files selected for processing (5)
apps/security-advisory-patches-portal/README.mdapps/security-advisory-patches-portal/backend/constants.balapps/security-advisory-patches-portal/backend/modules/file_storage/client.balapps/security-advisory-patches-portal/backend/service.balapps/security-advisory-patches-portal/webapp/src/context/AuthContext.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/security-advisory-patches-portal/backend/constants.bal
- apps/security-advisory-patches-portal/backend/modules/file_storage/client.bal
- apps/security-advisory-patches-portal/webapp/src/context/AuthContext.tsx
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
cloby99
reviewed
May 18, 2026
cloby99
reviewed
May 18, 2026
cloby99
reviewed
May 18, 2026
cloby99
reviewed
May 18, 2026
cloby99
reviewed
May 18, 2026
Apply cloby99 feedback: bump WSO2 copyright headers to 2026 across the portal backend and webapp, and use inferred-type const declarations in backend/constants.bal. Align README and Config.toml.local with securityPatchesUserRole naming.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/security-advisory-patches-portal/webapp/src/utils/fileService.ts`:
- Around line 34-37: getFileName currently returns the entire input when the
path ends with a trailing slash (e.g., "foo/bar/") because the last split
segment is empty; update getFileName to normalize the path before selecting the
final segment—either strip trailing slashes (e.g., remove trailing '/'
characters) or split and filter out empty segments (e.g., parts.filter(Boolean))
and then return the last non-empty element—refer to the getFileName function and
the parts variable to implement this change so inputs like "foo/bar/" yield
"bar".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8cf23ffd-0f29-4f9b-8b00-2b480cc08b7a
📒 Files selected for processing (34)
apps/security-advisory-patches-portal/README.mdapps/security-advisory-patches-portal/backend/Config.toml.localapps/security-advisory-patches-portal/backend/constants.balapps/security-advisory-patches-portal/backend/modules/authorization/authorization.balapps/security-advisory-patches-portal/backend/modules/authorization/constants.balapps/security-advisory-patches-portal/backend/modules/authorization/types.balapps/security-advisory-patches-portal/backend/modules/authorization/utils.balapps/security-advisory-patches-portal/backend/modules/file_storage/client.balapps/security-advisory-patches-portal/backend/modules/file_storage/constants.balapps/security-advisory-patches-portal/backend/modules/file_storage/file_storage.balapps/security-advisory-patches-portal/backend/modules/file_storage/types.balapps/security-advisory-patches-portal/backend/modules/file_storage/utils.balapps/security-advisory-patches-portal/backend/service.balapps/security-advisory-patches-portal/webapp/index.htmlapps/security-advisory-patches-portal/webapp/public/index.htmlapps/security-advisory-patches-portal/webapp/src/App.tsxapps/security-advisory-patches-portal/webapp/src/app/AppHandler.tsxapps/security-advisory-patches-portal/webapp/src/config/config.tsapps/security-advisory-patches-portal/webapp/src/constants/constants.tsapps/security-advisory-patches-portal/webapp/src/context/AuthContext.tsxapps/security-advisory-patches-portal/webapp/src/index.cssapps/security-advisory-patches-portal/webapp/src/index.tsxapps/security-advisory-patches-portal/webapp/src/layout/Header.tsxapps/security-advisory-patches-portal/webapp/src/slices/authSlice/auth.tsapps/security-advisory-patches-portal/webapp/src/slices/store.tsapps/security-advisory-patches-portal/webapp/src/theme.tsapps/security-advisory-patches-portal/webapp/src/types/images.d.tsapps/security-advisory-patches-portal/webapp/src/types/types.tsapps/security-advisory-patches-portal/webapp/src/utils/apiService.tsapps/security-advisory-patches-portal/webapp/src/utils/fileService.tsapps/security-advisory-patches-portal/webapp/src/utils/utils.tsapps/security-advisory-patches-portal/webapp/src/view/NotFound/NotFoundPage.tsxapps/security-advisory-patches-portal/webapp/src/view/PatchesPdf/PatchesPdfPage.tsxapps/security-advisory-patches-portal/webapp/src/view/RootLanding/RootLandingPage.tsx
✅ Files skipped from review due to trivial changes (2)
- apps/security-advisory-patches-portal/backend/modules/file_storage/constants.bal
- apps/security-advisory-patches-portal/webapp/public/index.html
🚧 Files skipped from review as they are similar to previous changes (28)
- apps/security-advisory-patches-portal/webapp/src/types/images.d.ts
- apps/security-advisory-patches-portal/webapp/src/view/RootLanding/RootLandingPage.tsx
- apps/security-advisory-patches-portal/backend/modules/file_storage/client.bal
- apps/security-advisory-patches-portal/backend/modules/file_storage/types.bal
- apps/security-advisory-patches-portal/webapp/src/index.tsx
- apps/security-advisory-patches-portal/webapp/index.html
- apps/security-advisory-patches-portal/backend/modules/authorization/utils.bal
- apps/security-advisory-patches-portal/webapp/src/slices/store.ts
- apps/security-advisory-patches-portal/webapp/src/types/types.ts
- apps/security-advisory-patches-portal/backend/Config.toml.local
- apps/security-advisory-patches-portal/webapp/src/theme.ts
- apps/security-advisory-patches-portal/README.md
- apps/security-advisory-patches-portal/webapp/src/view/NotFound/NotFoundPage.tsx
- apps/security-advisory-patches-portal/webapp/src/App.tsx
- apps/security-advisory-patches-portal/backend/modules/authorization/constants.bal
- apps/security-advisory-patches-portal/backend/modules/authorization/authorization.bal
- apps/security-advisory-patches-portal/webapp/src/view/PatchesPdf/PatchesPdfPage.tsx
- apps/security-advisory-patches-portal/webapp/src/config/config.ts
- apps/security-advisory-patches-portal/webapp/src/app/AppHandler.tsx
- apps/security-advisory-patches-portal/backend/constants.bal
- apps/security-advisory-patches-portal/webapp/src/constants/constants.ts
- apps/security-advisory-patches-portal/webapp/src/slices/authSlice/auth.ts
- apps/security-advisory-patches-portal/webapp/src/layout/Header.tsx
- apps/security-advisory-patches-portal/webapp/src/utils/apiService.ts
- apps/security-advisory-patches-portal/backend/service.bal
- apps/security-advisory-patches-portal/webapp/src/context/AuthContext.tsx
- apps/security-advisory-patches-portal/webapp/src/utils/utils.ts
- apps/security-advisory-patches-portal/backend/modules/file_storage/file_storage.bal
Removed CORS configuration from the security advisories backend service.
cloby99
reviewed
May 25, 2026
Comment on lines
+19
to
+24
| public type CustomJwtPayload record { | ||
| # User email | ||
| string email; | ||
| # User groups | ||
| string[] groups; | ||
| }; |
Member
There was a problem hiding this comment.
Suggested change
| public type CustomJwtPayload record { | |
| # User email | |
| string email; | |
| # User groups | |
| string[] groups; | |
| }; | |
| public type CustomJwtPayload record {| | |
| # User email | |
| string email; | |
| # User groups | |
| string[] groups; | |
| |}; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request introduces the initial implementation and documentation for the Security Advisory Patches Portal, including backend configuration, Azure File Share integration, and developer/user guides. The main changes set up the Ballerina backend to interact with Azure Files, define configuration and dependency management, and provide comprehensive documentation for users, publishers, and developers.
Backend Implementation and Configuration:
Ballerina.tomlfor package definition and versioning, andConfig.toml.localfor local Azure File Share configuration (account name, share name, credentials, and authorization method). [1] [2]Dependencies.tomlto manage Ballerina and Azure module dependencies, ensuring the backend can interact with Azure File Shares and handle HTTP requests.Azure File Share Integration:
modules/file_storage/client.balto configure and instantiate a Ballerina Azure File Service client, enabling secure and configurable access to Azure File Shares.Constants and Validation:
constants.bal, ensuring consistent error handling and secure path validation.modules/file_storage/constants.bal.Summary by CodeRabbit