Copilot commented on code in PR #1191: URL: https://github.com/apache/skywalking-banyandb/pull/1191#discussion_r3471596813
########## canopy/server/src/plugins/proxy.ts: ########## @@ -0,0 +1,168 @@ +/* + * Licensed to Apache Software Foundation (ASF) under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Apache Software Foundation (ASF) licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { request as undiciRequest, type Dispatcher } from 'undici'; +import type { FastifyInstance, FastifyRequest, FastifyReply } from 'fastify'; +import { checkEndpointSafe, SsrfError } from '../lib/ssrf.js'; +import { enforceRole } from '../lib/role.js'; +import type { Config } from '../config.js'; + +function buildUpstreamAuth(config: Config): string | undefined { + if (config.upstreamUsername && config.upstreamPassword) { + const encoded = Buffer.from(`${config.upstreamUsername}:${config.upstreamPassword}`).toString('base64'); + return `Basic ${encoded}`; + } + return undefined; +} + +function getSessionEndpoint(request: FastifyRequest, config: Config): string { + return request.session?.endpoint || config.banyandbTarget; +} + +async function forwardRequest( + upstreamBase: string, + upstreamPath: string, + request: FastifyRequest, + reply: FastifyReply, + config: Config, + upstreamAuth: string | undefined, +): Promise<void> { + const targetUrl = `${upstreamBase}${upstreamPath}`; + + // Re-check SSRF on every request (per-request DNS re-resolution) + try { + await checkEndpointSafe(upstreamBase, config.blockRfc1918); + } catch (err) { + if (err instanceof SsrfError) { + await reply.status(403).send({ error: 'ssrf_denied', message: err.message }); + return; + } + throw err; + } + + const headers: Record<string, string> = {}; + + // Forward relevant headers from client; skip hop-by-hop and let undici manage content-length + for (const [k, v] of Object.entries(request.headers)) { + const lower = k.toLowerCase(); + if (lower === 'host' || lower === 'connection' || lower === 'transfer-encoding' || lower === 'content-length') continue; + if (typeof v === 'string') headers[k] = v; + } + + // Attach upstream service credential if configured (attach-if-configured, global) + if (upstreamAuth) { + headers['authorization'] = upstreamAuth; + } + + // Re-serialize body: Fastify parses application/json into a JS object; undici requires string/Buffer. + // Skipping for bodyless methods avoids sending empty payloads on GET/HEAD/DELETE/OPTIONS. + let upstreamBody: string | undefined; + if (!['GET', 'HEAD', 'DELETE', 'OPTIONS'].includes(request.method) && request.body != null) { + upstreamBody = typeof request.body === 'string' ? request.body : JSON.stringify(request.body); + if (!headers['content-type']) headers['content-type'] = 'application/json'; + headers['content-length'] = String(Buffer.byteLength(upstreamBody)); + } + + let upstreamRes: Dispatcher.ResponseData; + try { + upstreamRes = await undiciRequest(targetUrl, { + method: request.method as Dispatcher.HttpMethod, + headers, + body: upstreamBody, + bodyTimeout: config.upstreamTimeoutMs, + headersTimeout: config.upstreamTimeoutMs, + // undici v7 request() does not follow redirects by default (no RedirectAgent/RedirectHandler + // configured), so 3xx responses are forwarded verbatim — the SSRF deny-narrow contract holds. + reset: true, // send Connection: close (no keep-alive pooling per proxy request) + }); + } catch (err: unknown) { + const code = (err as NodeJS.ErrnoException).code; + if (code === 'ECONNREFUSED' || code === 'ENOTFOUND') { + await reply.status(502).send({ + error: 'upstream_error', + message: `Cannot connect to upstream: ${upstreamBase}`, + }); + return; + } + if (code === 'UND_ERR_HEADERS_TIMEOUT' || code === 'UND_ERR_BODY_TIMEOUT') { + await reply.status(504).send({ + error: 'upstream_timeout', + message: `Upstream request timed out after ${config.upstreamTimeoutMs}ms`, + }); + return; + } + throw err; + } + + // Handle upstream 401 distinctly — wrap in a specific error envelope + if (upstreamRes.statusCode === 401) { + // Drain the body + await upstreamRes.body.dump(); + await reply.status(401).send({ + error: 'upstream_auth_error', + message: 'The target cluster rejected the configured service credential. Check BANYANDB_UPSTREAM_USERNAME/PASSWORD or the endpoint.', + upstream: upstreamBase, + }); + return; + } + + // Upstream 5xx → wrap as 502 + if (upstreamRes.statusCode >= 500) { + await upstreamRes.body.dump(); + await reply.status(502).send({ + error: 'upstream_error', + message: `Upstream returned ${upstreamRes.statusCode}`, + }); + return; + } + + // Forward the response + const forwardHeaders: Record<string, string | string[]> = {}; + for (const [k, v] of Object.entries(upstreamRes.headers)) { + const lower = k.toLowerCase(); + if (lower === 'transfer-encoding' || lower === 'connection') continue; + if (v !== undefined) forwardHeaders[k] = v as string | string[]; + } + reply.raw.writeHead(upstreamRes.statusCode, forwardHeaders); + await upstreamRes.body.pipe(reply.raw); +} + +export async function registerProxy(app: FastifyInstance, config: Config): Promise<void> { + const upstreamAuth = buildUpstreamAuth(config); + + // Auth middleware for all proxy routes + async function requireAuth(request: FastifyRequest, reply: FastifyReply) { + if (!request.session?.user) { + await reply.status(401).send({ error: 'unauthenticated', message: 'Login required' }); + } + } Review Comment: `requireAuth` sends a 401 but doesn’t return/stop the request pipeline. That allows the main handler to continue executing (including attempting upstream proxying) after an unauthenticated response is already sent, which can cause unintended upstream traffic and write-after-end errors. Return the reply to short-circuit the request. ########## canopy/server/src/lib/role.ts: ########## @@ -0,0 +1,35 @@ +/* + * Licensed to Apache Software Foundation (ASF) under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Apache Software Foundation (ASF) licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import type { FastifyRequest, FastifyReply } from 'fastify'; + +const READ_METHODS = new Set(['GET', 'HEAD', 'OPTIONS']); + +export function isReadOnlySession(request: FastifyRequest): boolean { + return request.session?.role === 'readonly'; +} + +export async function enforceRole(request: FastifyRequest, reply: FastifyReply): Promise<void> { + if (!READ_METHODS.has(request.method) && isReadOnlySession(request)) { + await reply.status(403).send({ + error: 'forbidden', + message: 'Your account has read-only access. This operation requires an admin role.', + }); + } +} Review Comment: `enforceRole` sends a 403 for read-only sessions but doesn’t return. As a preHandler, that means the downstream proxy handler can still run and forward the mutation upstream (or trigger write-after-end errors). Return the reply to stop processing when access is denied. ########## canopy/server/src/routes/auth.ts: ########## @@ -0,0 +1,125 @@ +/* + * Licensed to Apache Software Foundation (ASF) under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Apache Software Foundation (ASF) licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import bcrypt from 'bcryptjs'; +import type { FastifyInstance, FastifyRequest, FastifyReply } from 'fastify'; +import { request as undiciRequest } from 'undici'; + +import type { Config } from '../config.js'; + +interface LoginBody { + username: string; + password: string; + endpoint: string; +} + +function normalizeEndpoint(raw: string): string { + // Accept bare host:port and normalize to http:// + // Leave any other scheme (ftp://, etc.) unchanged so isValidHttpUrl rejects it. + if (/^https?:\/\//i.test(raw)) return raw; + if (/^[a-z][a-z0-9+\-.]*:\/\//i.test(raw)) return raw; + return `http://${raw}`; +} + +function isValidHttpUrl(s: string): boolean { + try { + const u = new URL(s); + return u.protocol === 'http:' || u.protocol === 'https:'; + } catch { + return false; + } +} + +async function fetchBanyanVersion(target: string): Promise<string | null> { + try { + const res = await undiciRequest(`${target}/api/v1/common/api/version`, { + method: 'GET', + headersTimeout: 3000, + bodyTimeout: 3000, + }); + if (res.statusCode === 200) { + const data = await res.body.json() as { version?: { version?: string } }; + return data?.version?.version ?? null; + } + await res.body.dump(); + } catch { + // best-effort + } + return null; +} + +export async function registerAuth(app: FastifyInstance, config: Config): Promise<void> { + // Unauthenticated — exposes BanyanDB version for the login page before credentials are known. + // Registered before the authenticated /api/* proxy so Fastify matches this route first. + app.get('/api/meta', async (_request: FastifyRequest, reply: FastifyReply) => { + const banyanVersion = await fetchBanyanVersion(config.banyandbTarget); + return reply.send({ banyanVersion }); + }); + + app.post<{ Body: LoginBody }>('/auth/login', async (request: FastifyRequest, reply: FastifyReply) => { + const { username, password, endpoint } = request.body as LoginBody; + + if (!username || !password || !endpoint) { + return reply.status(400).send({ error: 'bad_request', message: 'username, password, and endpoint are required' }); + } + + const normalizedEndpoint = normalizeEndpoint(endpoint); + if (!isValidHttpUrl(normalizedEndpoint)) { + return reply.status(400).send({ error: 'bad_endpoint', message: 'Endpoint must be a valid http(s) URL or host:port' }); + } + + // Find user (constant-time compare to prevent timing attacks) + const user = config.users.find(u => u.username === username); + + // Always run bcrypt compare even if user not found (timing-safe) + const hashToCheck = user?.passwordHash ?? '$2b$10$invalidhashpaddingtomakeitconstanttimexxx'; + let passwordOk = false; + try { + passwordOk = await bcrypt.compare(password, hashToCheck); + } catch { + passwordOk = false; + } + + if (!user || !passwordOk) { + return reply.status(401).send({ error: 'invalid_credentials', message: 'Invalid username or password' }); + } + + const banyanVersion = await fetchBanyanVersion(normalizedEndpoint); + Review Comment: `fetchBanyanVersion()` is executed against the user-supplied endpoint immediately after credential validation, but before any SSRF deny-narrow check is applied. This means a user who can authenticate can still trigger server-side requests to denied targets (e.g., cloud metadata) during login. Apply the same SSRF check used by the proxy before calling `fetchBanyanVersion()`. ########## canopy/server/src/routes/auth.ts: ########## @@ -0,0 +1,125 @@ +/* + * Licensed to Apache Software Foundation (ASF) under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Apache Software Foundation (ASF) licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import bcrypt from 'bcryptjs'; +import type { FastifyInstance, FastifyRequest, FastifyReply } from 'fastify'; +import { request as undiciRequest } from 'undici'; + +import type { Config } from '../config.js'; + +interface LoginBody { + username: string; + password: string; + endpoint: string; +} + +function normalizeEndpoint(raw: string): string { + // Accept bare host:port and normalize to http:// + // Leave any other scheme (ftp://, etc.) unchanged so isValidHttpUrl rejects it. + if (/^https?:\/\//i.test(raw)) return raw; + if (/^[a-z][a-z0-9+\-.]*:\/\//i.test(raw)) return raw; + return `http://${raw}`; +} + +function isValidHttpUrl(s: string): boolean { + try { + const u = new URL(s); + return u.protocol === 'http:' || u.protocol === 'https:'; + } catch { + return false; + } +} Review Comment: `normalizeEndpoint()` currently returns the raw URL (or `http://host:port`) without canonicalizing to an origin. That allows values like `http://host/basepath` to be stored into the session and later concatenated with `/api/*` paths, producing malformed upstream URLs. It also allows embedding userinfo (credentials) in the endpoint. Canonicalize to `u.origin` for http(s) URLs, and make `isValidHttpUrl()` reject userinfo. ########## canopy/server/package.json: ########## @@ -0,0 +1,39 @@ +{ + "name": "canopy-server", + "version": "0.1.0", + "private": true, + "type": "module", + "engines": { "node": "24.6.0" }, + "main": "dist/index.js", + "scripts": { + "dev": "node --loader ts-node/esm src/index.ts", + "build": "tsc", + "typecheck": "tsc --noEmit", + "lint": "eslint src test", + "test": "vitest run", + "test:watch": "vitest" + }, + "dependencies": { + "fastify": "^5.4.0", + "@fastify/static": "^9.1.3", + "@fastify/secure-session": "^8.1.1", + "@fastify/cookie": "^11.0.2", + "undici": "^7.10.0", + "bcryptjs": "^3.0.2", + "argon2": "^0.43.0", + "js-yaml": "^4.1.0", + "canopy-shared": "*" + }, Review Comment: `argon2` is included as a production dependency but isn’t referenced anywhere in the server code. Since `argon2` is a native addon with an install script, keeping it unused increases install/CI fragility and supply-chain surface area. Remove it until it’s actually needed (bcryptjs is what the implementation uses today). ########## canopy/web/src/pages/LoginPage.tsx: ########## @@ -0,0 +1,287 @@ +/* + * Licensed to Apache Software Foundation (ASF) under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Apache Software Foundation (ASF) licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import React, { useState, useEffect } from 'react'; + +import { CanopyMark } from '../components/CanopyMark.js'; +import { useAuth } from '../auth/AuthContext.js'; +import { IconShield, IconViewer } from '../components/icons.js'; + +type Role = 'admin' | 'readonly'; +type Status = 'idle' | 'connecting' | 'connected' | 'failed'; + +interface FieldProps { + label: string; + required?: boolean; + value: string; + onChange: (v: string) => void; + type?: string; + placeholder?: string; + mono?: boolean; + error?: string; + eye?: boolean; + eyeOn?: boolean; + onEye?: () => void; + onSubmit?: () => void; +} + +function EyeIcon({ off }: { off: boolean }) { + return ( + <svg width="16" height="16" viewBox="0 0 24 24" fill="none" stroke="currentColor" strokeWidth="1.7" strokeLinecap="round" strokeLinejoin="round"> + {off ? ( + <> + <path d="M9.9 4.24A9.12 9.12 0 0 1 12 4c7 0 10 8 10 8a18.5 18.5 0 0 1-2.16 3.19m-6.72-1.07a3 3 0 1 1-4.24-4.24" /> + <path d="M6.61 6.61A18.5 18.5 0 0 0 2 12s3 8 10 8a9.12 9.12 0 0 0 5.39-1.61" /> + <path d="m2 2 20 20" /> + </> + ) : ( + <> + <path d="M2 12s3-8 10-8 10 8 10 8-3 8-10 8-10-8-10-8Z" /> + <circle cx="12" cy="12" r="3" /> + </> + )} + </svg> + ); +} + + +function Field({ label, required, value, onChange, type = 'text', placeholder, mono, error, eye, eyeOn, onEye, onSubmit }: FieldProps) { + const fieldId = 'f-' + label.toLowerCase().replace(/\s+/g, '-'); + return ( + <div className={'f-field' + (error ? ' has-error' : '')}> + <label className="f-label" htmlFor={fieldId}> + {label} + {required && <span className="f-req">*</span>} + </label> + <div className="f-inputwrap"> + <input + id={fieldId} + className={'f-input' + (mono ? ' mono' : '') + (eye ? ' has-eye' : '')} + type={type} + value={value} + placeholder={placeholder} + spellCheck={false} + onChange={(e) => onChange(e.target.value)} + onKeyDown={(e) => { if (e.key === 'Enter' && onSubmit) onSubmit(); }} + /> + {eye && ( + <button type="button" className="f-eye" tabIndex={-1} onClick={onEye} aria-label="Toggle visibility"> + <EyeIcon off={!eyeOn} /> + </button> Review Comment: The password visibility toggle button is removed from the tab order via `tabIndex={-1}`, which prevents keyboard-only users from reaching it. Since this control affects password entry usability, it should be keyboard accessible (default tabIndex) and rely on proper focus styles instead. -- 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]
