Skip to content

Require secure transport for Zhihu upstream #517

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
GOLDKUN wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Require secure transport for Zhihu upstream #517

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
dcca7ea
Require secure transport for Zhihu upstream
Sep 2, 2026
814b475
Allow loopback HTTP for local service smoke tests
Sep 2, 2026
480faf2
Keep Zhihu upstream transport strictly secure
Sep 2, 2026
3a4fb51
Trust generated smoke test certificate explicitly
Sep 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 21 additions & 6 deletions scripts/service-package-smoke.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
#!/usr/bin/env node
import { spawn } from "node:child_process";
import http from "node:http";
import { execFileSync, spawn } from "node:child_process";
import https from "node:https";
import http2 from "node:http2";
import fs from "node:fs";
import net from "node:net";
Expand Down Expand Up @@ -31,7 +31,7 @@ const mock = await startMockUpstream();
const addr = `127.0.0.1:${await freePort()}`;
const daemon = spawn(octobusBin, ["serve", "--addr", addr, "--data-dir", dataDir], {
cwd: repoRoot,
env: { ...process.env, OCTOBUS_ADDR: addr, OCTOBUS_DATA_DIR: dataDir },
env: { ...process.env, OCTOBUS_ADDR: addr, OCTOBUS_DATA_DIR: dataDir, NODE_EXTRA_CA_CERTS: mock.caFile },
stdio: ["ignore", "pipe", "pipe"],
});

Expand Down Expand Up @@ -280,7 +280,18 @@ function requiredValue(args, index, flag) {
async function startMockUpstream() {
let hitCount = 0;
const requests = [];
const server = http.createServer((req, res) => {
const tlsDir = fs.mkdtempSync(path.join(os.tmpdir(), "octobus-smoke-tls."));
const keyPath = path.join(tlsDir, "key.pem");
const certPath = path.join(tlsDir, "cert.pem");
execFileSync("openssl", [
"req", "-x509", "-newkey", "rsa:2048", "-nodes",
"-keyout", keyPath, "-out", certPath, "-days", "1",
"-subj", "/CN=127.0.0.1", "-addext", "subjectAltName=IP:127.0.0.1",
], { stdio: "ignore" });
const server = https.createServer({
key: fs.readFileSync(keyPath),
cert: fs.readFileSync(certPath),
}, (req, res) => {
hitCount += 1;
const chunks = [];
req.on("data", (chunk) => chunks.push(chunk));
Expand Down Expand Up @@ -341,8 +352,12 @@ async function startMockUpstream() {
get requests() {
return requests;
},
baseURL: `http://127.0.0.1:${address.port}`,
close: () => new Promise((resolve) => server.close(resolve)),
baseURL: `https://127.0.0.1:${address.port}`,
caFile: certPath,
close: () => new Promise((resolve) => server.close(() => {
fs.rmSync(tlsDir, { recursive: true, force: true });
resolve();
})),
};
}

Expand Down
13 changes: 7 additions & 6 deletions services/zhihu__open-api/config.schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
"baseUrl": {
"type": "string",
"format": "uri",
"pattern": "^https://",
"default": "https://developer.zhihu.com",
"description": "Zhihu Open Platform root URL."
},
Expand All @@ -28,19 +29,19 @@
"description": "Optional additional HTTP headers. Core auth, JSON, and trace headers are set by the service."
},
"skipTlsVerify": {
"type": "boolean",
"const": false,
"default": false,
"description": "Skip TLS certificate verification for private testing."
"description": "TLS certificate verification is mandatory."
},
"tlsInsecureSkipVerify": {
"type": "boolean",
"const": false,
"default": false,
"description": "Legacy alias for skipTlsVerify."
"description": "Legacy alias; TLS certificate verification is mandatory."
},
"insecureSkipVerify": {
"type": "boolean",
"const": false,
"default": false,
"description": "Legacy alias for skipTlsVerify."
"description": "Legacy alias; TLS certificate verification is mandatory."
}
}
}
44 changes: 23 additions & 21 deletions services/zhihu__open-api/src/zhihu-open-api.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { GrpcError, createTlsDispatcher, grpcCodeFor, normalizeTimeoutMs } from '@chaitin-ai/octobus-sdk';
import { GrpcError, grpcCodeFor, normalizeTimeoutMs } from '@chaitin-ai/octobus-sdk';

// ---------------------------------------------------------------------------
// Method table
Expand Down Expand Up @@ -26,7 +26,6 @@ export const METHODS = {
// ---------------------------------------------------------------------------
const DEFAULT_BASE_URL = 'https://developer.zhihu.com';
const DEFAULT_TIMEOUT_MS = 10_000;
const insecureTlsDispatcher = createTlsDispatcher(true);

const SEARCH_DB_VALUES = ['all', 'realtime', 'static'];
const SCOPE_VALUES = ['all', 'created', 'subscribed'];
Expand Down Expand Up @@ -261,8 +260,8 @@ const normalizeBaseUrl = (value) => {
} catch {
throw errorWithCode('INVALID_ARGUMENT', 'baseUrl must be a valid URL');
}
if (url.protocol !== 'https:' && url.protocol !== 'http:') {
throw errorWithCode('INVALID_ARGUMENT', 'baseUrl must use http or https');
if (url.protocol !== 'https:') {
throw errorWithCode('INVALID_ARGUMENT', 'baseUrl must use https');
}
url.pathname = url.pathname.replace(/\/+$/, '');
url.search = '';
Expand All @@ -284,28 +283,32 @@ const resolveCallContext = (ctx = {}) => ({
req: ctx.req ?? ctx.request ?? {},
});

const resolveSettings = (ctx = {}) => ({
baseUrl: normalizeBaseUrl(ctx.config?.baseUrl ?? ctx.bindings?.baseUrl),
timeoutMs: normalizeTimeoutMs(
firstDefined(ctx.config?.timeoutMs, ctx.config?.timeout_ms, ctx.bindings?.timeoutMs, ctx.limits?.timeoutMs),
DEFAULT_TIMEOUT_MS,
),
headers: (ctx.config?.headers ?? ctx.bindings?.headers) ?? {},
dispatcher: firstDefined(
const resolveSettings = (ctx = {}) => {
const tlsInsecure = [
ctx.config?.skipTlsVerify,
ctx.config?.tlsInsecureSkipVerify,
ctx.config?.insecureSkipVerify,
ctx.bindings?.skipTlsVerify,
ctx.bindings?.tlsInsecureSkipVerify,
ctx.bindings?.insecureSkipVerify,
) === true
? insecureTlsDispatcher
: undefined,
accessSecret: requiredString(ctx.secret?.accessSecret ?? ctx.secret?.access_secret, 'accessSecret'),
oauthToken: asString(ctx.secret?.oauthToken ?? ctx.secret?.oauth_token),
fetchImpl: ctx.fetchImpl ?? globalThis.fetch,
meta: ctx.meta ?? {},
});
].some((value) => value === true);
if (tlsInsecure) {
Comment thread
monkeyscan[bot] marked this conversation as resolved.
throw errorWithCode('INVALID_ARGUMENT', 'TLS certificate verification cannot be disabled');
}
return {
baseUrl: normalizeBaseUrl(ctx.config?.baseUrl ?? ctx.bindings?.baseUrl),
timeoutMs: normalizeTimeoutMs(
firstDefined(ctx.config?.timeoutMs, ctx.config?.timeout_ms, ctx.bindings?.timeoutMs, ctx.limits?.timeoutMs),
DEFAULT_TIMEOUT_MS,
),
headers: (ctx.config?.headers ?? ctx.bindings?.headers) ?? {},
dispatcher: undefined,
Comment thread
monkeyscan[bot] marked this conversation as resolved.
accessSecret: requiredString(ctx.secret?.accessSecret ?? ctx.secret?.access_secret, 'accessSecret'),
oauthToken: asString(ctx.secret?.oauthToken ?? ctx.secret?.oauth_token),
fetchImpl: ctx.fetchImpl ?? globalThis.fetch,
meta: ctx.meta ?? {},
};
};

const resolveOauthToken = (settings, request = {}) => (
asString(request.oauth_token ?? request.oauthToken) || settings.oauthToken
Expand Down Expand Up @@ -515,7 +518,6 @@ export const _test = {
errorWithCode,
firstDefined,
hasOwn,
insecureTlsDispatcher,
isTimeoutError,
logInfo,
mapErrorCode,
Expand Down
23 changes: 16 additions & 7 deletions services/zhihu__open-api/test/zhihu-open-api.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,8 @@ test('requires an Access Secret before any request is issued', async () => {
(error) => error.code === grpcStatus.INVALID_ARGUMENT && /accessSecret is required/.test(error.message),
);
assert.throws(
() => _test.resolveSettings({ config: { baseUrl: 'ftp://example' }, secret: { accessSecret: 'a' } }),
/baseUrl must use http or https/,
() => _test.resolveSettings({ config: { baseUrl: 'http://example' }, secret: { accessSecret: 'a' } }),
/baseUrl must use https/,
);
assert.throws(
() => _test.normalizeBaseUrl('not a url'),
Expand Down Expand Up @@ -492,31 +492,40 @@ test('parseResponse maps non-OK HTTP with and without a Message field', async ()
assert.deepEqual(ok, { data: { a: 1 } });
});

test('respects config timeouts, custom headers, TLS flags, and legacy aliases', async () => {
test('respects config timeouts, custom headers, and legacy aliases', async () => {
let captured;
globalThis.fetch = async (url, init) => {
captured = { url, init };
return response(okData({}));
};
const result = await handlers[METHODS.GET_HOT_LIST]({
config: {
baseUrl: 'http://localhost:18082',
baseUrl: 'https://localhost:18082',
timeout_ms: 3100,
headers: { 'X-Custom': 'value' },
skipTlsVerify: true,
},
secret: { access_secret: 'legacy-secret' },
meta: { instance_id: 'inst', request_id: 'req' },
request: { limit: 2 },
});
assert.deepEqual(result.data, {});
assert.equal(captured.url, 'http://localhost:18082/api/v1/content/hot_list?Limit=2');
assert.equal(captured.url, 'https://localhost:18082/api/v1/content/hot_list?Limit=2');
assert.equal(captured.init.headers['X-Custom'], 'value');
assert.equal(captured.init.headers['x-engine-instance'], 'inst');
assert.equal(captured.init.headers['x-request-id'], 'req');
assert.equal(captured.init.headers.Authorization, 'Bearer legacy-secret');
assert.equal(captured.init.dispatcher, _test.insecureTlsDispatcher);
assert.equal(captured.init.dispatcher, undefined);
assert.ok(captured.init.signal instanceof AbortSignal);
assert.throws(
() => _test.resolveSettings({ config: { baseUrl: 'https://example', skipTlsVerify: true }, secret: { accessSecret: 'a' } }),
/TLS certificate verification cannot be disabled/,
);
for (const alias of ['tlsInsecureSkipVerify', 'insecureSkipVerify']) {
assert.throws(
() => _test.resolveSettings({ config: { baseUrl: 'https://example', [alias]: true }, secret: { accessSecret: 'a' } }),
/TLS certificate verification cannot be disabled/,
);
}

@monkeyscan monkeyscan Bot Sep 2, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

「前置 false + 后置 legacy true」组合缺少回归测试，新增用例在旧逻辑下同样通过

本次将 TLS 守卫从 firstDefined(...) === true 改为数组的 .some(v => v === true)，正是为了修复历史确认的问题：当靠前标志显式为 false 而靠后的 legacy 别名为 true 时（例如 config: { skipTlsVerify: false, tlsInsecureSkipVerify: true }），firstDefined 会直接返回 false，守卫被绕过，本应抛出的「TLS certificate verification cannot be disabled」被静默跳过。但新增的测试仅覆盖「单个标志为 true」的场景（skipTlsVerify: true、tlsInsecureSkipVerify: true、insecureSkipVerify: true 各自单独出现）。在旧的 firstDefined 实现下，这些场景中靠前的标志都是 undefined（未定义），firstDefined 会继续向后取到 true 并照常抛出，因此这些新用例在修复前后行为完全相同，无法区分 bug 与修复。真正需要保护的回归场景（前位 false、后位 true 的组合）没有任何断言覆盖，将来若有人改回 firstDefined 或等价逻辑，测试套件依然会全部通过。

Problem code:

Changed code at services/zhihu__open-api/test/zhihu-open-api.test.js:523-528

Recommendation:
补充一个覆盖「前位 false + 后位 legacy true」组合的断言，锁定本次 .some() 修复的行为，例如：assert.throws(() => _test.resolveSettings({ config: { baseUrl: 'https://example', skipTlsVerify: false, tlsInsecureSkipVerify: true }, secret: { accessSecret: 'a' } }), /TLS certificate verification cannot be disabled/); 同理可对 bindings 层级与 insecureSkipVerify 各补一组。

Suggested diff:

diff --git a/services/zhihu__open-api/test/zhihu-open-api.test.js b/services/zhihu__open-api/test/zhihu-open-api.test.js
--- a/services/zhihu__open-api/test/zhihu-open-api.test.js
+++ b/services/zhihu__open-api/test/zhihu-open-api.test.js
@@ -520,6 +520,14 @@ test('respects config timeouts, custom headers, and legacy aliases', async () =>
   for (const alias of ['tlsInsecureSkipVerify', 'insecureSkipVerify']) {
     assert.throws(
       () => _test.resolveSettings({ config: { baseUrl: 'https://example', [alias]: true }, secret: { accessSecret: 'a' } }),
       /TLS certificate verification cannot be disabled/,
     );
   }
+  // 回归用例：前位显式 false + 后位 legacy true 在旧的 firstDefined 实现下会被绕过。
+  assert.throws(
+    () => _test.resolveSettings({ config: { baseUrl: 'https://example', skipTlsVerify: false, tlsInsecureSkipVerify: true }, secret: { accessSecret: 'a' } }),
+    /TLS certificate verification cannot be disabled/,
+  );

const settings = _test.resolveSettings({
config: { baseUrl: 'https://x', timeoutMs: 2000, headers: { a: 'b' } },
secret: { accessSecret: 's' },
Expand Down
Loading