Copilot commented on code in PR #11142:
URL: https://github.com/apache/gravitino/pull/11142#discussion_r3263477529
##########
web-v2/web/src/lib/auth/providers/oidc.js:
##########
@@ -20,6 +20,31 @@
import { BaseOAuthProvider } from './base'
import { UserManager, WebStorageStateStore } from 'oidc-client-ts'
+function getOidcAppBasePath() {
+ const basePath = process.env.NEXT_PUBLIC_BASE_PATH || ''
+
+ if (!basePath || basePath === '/') {
+ return ''
+ }
+
+ return basePath.replace(/\/$/, '')
+}
+
+function buildOidcUrl(pathname) {
+ return `${window.location.origin}${getOidcAppBasePath()}${pathname}`
+}
+
+function isOidcSecureOrigin() {
+ if (typeof window === 'undefined') {
+ return true
+ }
Review Comment:
`isOidcSecureOrigin()` returns `true` when `window` is undefined, but
`initialize()` unconditionally uses `window.location`/`window.localStorage`
later. If `initialize()` is ever invoked during SSR or in a non-browser
environment, this will pass the security check and then crash with a
ReferenceError. Consider explicitly rejecting non-browser environments at the
start of `initialize()` (or have `isOidcSecureOrigin()` return `false` when
`window` is undefined).
##########
web-v2/web/src/lib/auth/providers/oidc.test.js:
##########
@@ -30,11 +30,34 @@ vi.mock('oidc-client-ts', () => ({
describe('OidcOAuthProvider', () => {
let provider
let mockUserManager
+ let originalBasePath
+
+ const DEFAULT_SECURE_LOCATION = {
+ origin: 'https://localhost:3000',
+ protocol: 'https:',
+ hostname: 'localhost'
+ }
+
+ const setWindowLocation = ({ origin, protocol, hostname }) => {
+ Object.defineProperty(window, 'location', {
+ configurable: true,
+ value: {
+ ...window.location,
+ origin,
+ protocol,
+ hostname
+ }
+ })
+ }
Review Comment:
The helper replaces `window.location` with a plain object but never restores
the original JSDOM `Location`. This can leak into other test files (and
spreading `...window.location` won’t preserve non-enumerable `Location` members
like `href`, `assign`, etc.). Consider capturing the original `window.location`
once and restoring it in `afterEach/afterAll`, or use a dedicated stub/mock
utility that keeps the real `Location` intact.
##########
web-v2/web/src/lib/auth/providers/oidc.js:
##########
@@ -31,6 +56,12 @@ export class OidcOAuthProvider extends BaseOAuthProvider {
async initialize(config) {
this.config = config
+ if (!isOidcSecureOrigin()) {
+ throw new Error(
+ `OIDC login requires the UI to run on HTTPS or localhost. Current
origin: ${window.location.origin}`
+ )
+ }
Review Comment:
This adds a hard failure for running the UI on non-localhost HTTP origins.
That behavior isn’t mentioned in the PR description/linked issue and could
break existing (non-HTTPS) internal deployments. If this restriction is
intentional, please update the PR description (and ideally user docs) to call
it out; otherwise consider making it configurable or soft-failing (e.g.,
warning) instead of throwing during initialization.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
