Review/full workspace 2026 04 18 by gudaoxuri · Pull Request #996 · ideal-world/bios

gudaoxuri · 2026-04-18T03:33:48Z

No description provided.

- request_helper::parse_forwarded_ip no longer requires the 'Forwarded: ' header-line prefix; it now accepts either the raw header value (the form returned by poem's Request::headers().get(FORWARDED)) or a full header line. Previously the production caller in try_get_real_ip_from_req passed the raw value, so the Forwarded header branch never matched and silently fell through to X-Forwarded-For. Extended the doctest to cover both forms. - Fix typo in Cargo.toml feature comment (remvoe -> remove).

sea_orm TryGetable::try_get_by trait impls with a non-fatal TryGetError::DbErr return. These trait methods are dispatched by sea_orm when callers access columns by numeric index; a panic there would crash the whole process. BIOS currently only calls try_get with column names, so behaviour is unchanged in production but no longer explosive. - spi-conf tests: stop logging generated passwords in full. Four info! lines across spi_conf_api_test.rs, spi_conf_listener_test.rs and spi_conf_nacos_compatible_test.rs were emitting 'username: X, password: Y' plaintext in CI logs. Now only the password length is logged. - iam iam_config.rs: drop the hard-coded LDAP bind password LDAP without overriding the default were silently binding with this shipped credential; the default is now an empty string so operators must configure iam.ldap.bind_password explicitly.

…& block_on auth: - parsing_base_ak: webhook branch now enforces past-expiry window (previously only rejected future timestamps, leaving leaked historical signatures valid forever) [P0] - auth_res_serv::get_apis_json: converted to async fn and removed futures::executor::block_on call inside an async handler path, eliminating potential executor-thread deadlock [P1] - auth_crypto_serv::decrypt_req: replaced .expect("sm keys none") with a proper 500 error when SM2 keys are not initialized [P1] iam: - iam_key_cache_serv::add_aksk: stopped logging plaintext SK at trace level; now logs only ak and sk.len() [P0 credential leak] - iam_key_cache_serv::{add,delete}_token_by_token: stopped logging plaintext session tokens; logs only token length [P1] - iam_initializer: initial sys-admin password is no longer written into info! logs; caller still receives it for provisioning [P1] - iam_cert_oauth2_spi_github::get_access_token: moved client_secret from URL query string to application/x-www-form-urlencoded request body to prevent secret exposure in access/proxy logs [P1] Compile verified: cargo check -p bios-iam --lib cargo check -p bios-auth --lib --features web-server

Copilot

Pull request overview

This PR focuses on hardening Auth/IAM security and improving runtime safety by reducing credential leakage in logs, removing blocking block_on in async paths, and replacing panics with proper errors.

Changes:

Remove/avoid logging sensitive credentials (bootstrap passwords, tokens, AK/SK, CI test passwords) and reduce exposure of OAuth secrets in URLs.
Convert auth_res_serv::get_apis_json to async to avoid block_on in Tokio contexts; update several callers/tests accordingly.
Improve robustness by adding webhook replay-window checks and replacing panic!/expect with error returns in library code.

Reviewed changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
backend/supports/iam/src/iam_initializer.rs	Stops logging the initial sys-admin password; keeps returning it to the caller.
backend/supports/iam/src/iam_config.rs	Removes hardcoded LDAP default password (now empty by default).
backend/supports/iam/src/basic/serv/oauth2_spi/iam_cert_oauth2_spi_github.rs	Moves GitHub OAuth `client_secret` from URL query to form body.
backend/supports/iam/src/basic/serv/iam_key_cache_serv.rs	Prevents plaintext token/SK leakage in trace logs by logging only lengths.
backend/supports/auth/tests/test_auth_init.rs	Updates integration test to await async `get_apis_json`.
backend/supports/auth/src/serv/auth_res_serv.rs	Makes `get_apis_json` async and removes `block_on(fetch_public_key())`.
backend/supports/auth/src/serv/auth_kernel_serv.rs	Adds webhook request expiry check to reduce replay risk.
backend/supports/auth/src/serv/auth_crypto_serv.rs	Replaces panic on missing SM keys with an explicit internal error.
backend/supports/auth/src/api/auth_kernel_api.rs	Updates API handler to await async `get_apis_json`.
backend/spi/spi-stats/src/stats_enumeration.rs	Replaces `panic!("not implemented")` with `Err(...)` in `TryGetable::try_get_by`.
backend/spi/spi-conf/tests/spi_conf_nacos_compatible_test.rs	Avoids logging plaintext password in test output.
backend/spi/spi-conf/tests/spi_conf_listener_test.rs	Avoids logging plaintext password in test output.
backend/spi/spi-conf/tests/spi_conf_api_test.rs	Avoids logging plaintext password in test output (two locations).
backend/basic/src/helper/request_helper.rs	Makes `parse_forwarded_ip` accept raw Forwarded header values and improves docs/examples.
backend/basic/Cargo.toml	Minor typo fix and formatting of dependency/features lists.
REVIEW_AUTH_IAM_2026-04-18.md	Adds an audit/report document describing security/perf fixes and remaining suggestions.
.github/skills/review/SKILL.md	Adds a repository review guideline document.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-18T03:37:21Z

+        // GitHub OAuth App 的 client_id/client_secret/code 均由 GitHub 颁发，
+        // 字符集为 URL-safe，不包含 '&' 或 '='，可直接拼接为表单主体。
+        let form_body = format!("client_id={ak}&client_secret={sk}&code={code}");
+        let result = funs.web_client().post_to_obj::<Value>("https://github.com/login/oauth/access_token", &form_body, headers).await?;


The request body is built via string interpolation (client_id=...&client_secret=...&code=...). For application/x-www-form-urlencoded, values should be percent-encoded; manually concatenating can break if any value contains reserved characters (and the comment claiming they never contain &/= is brittle). Please build the form body using a proper URL-encoding utility (e.g., url::form_urlencoded) and consider removing or weakening the assumption in the comment.

Copilot · 2026-04-18T03:37:21Z

@@ -45,7 +45,7 @@ pub fn get_apis_json() -> TardisResult<Value> {
    } else {
        HashMap::new()
    };
-    let pub_key = block_on(auth_crypto_serv::fetch_public_key())?;
+    let pub_key = auth_crypto_serv::fetch_public_key().await?;
    TardisFuns::json.obj_to_json(&ServConfig {


get_apis_json is now async, but there is still at least one synchronous call site in the workspace that will no longer compile: backend/gateways/spacegate-plugins/src/plugin/auth.rs:266 calls auth_res_serv::get_apis_json() without .await. Please update that call site (it is already inside an async fn req) or provide a temporary sync wrapper if needed for non-async consumers.

Copilot · 2026-04-18T03:37:22Z

+            // Deployments must set `iam.ldap.bind_password` explicitly; this default is intentionally empty.
+            bind_password: String::new(),


Setting IamLdapConfig::default().bind_password to an empty string can unintentionally allow an admin bind with an empty password (the bind logic checks req.pw == config.bind_password). This is a security risk if LDAP is enabled but the password is not configured. Consider enforcing bind_password to be non-empty at startup/config load (fail fast), or changing the LDAP bind logic to reject admin binds when bind_password is empty (e.g., treat empty as “admin bind disabled”).

Suggested change

// Deployments must set `iam.ldap.bind_password` explicitly; this default is intentionally empty.

bind_password: String::new(),

// Use a non-empty sentinel so an omitted configuration cannot accidentally allow

// an admin bind with an empty password. Deployments must override this value via

// `iam.ldap.bind_password`.

bind_password: "__CONFIGURE_LDAP_BIND_PASSWORD__".to_string(),

gudaoxuri added 5 commits April 18, 2026 09:46

feat: add comprehensive code review guidelines for BIOS repository

ddd348d

fix(iam): improve logging and security measures in IAM services

953bf91

Copilot AI review requested due to automatic review settings April 18, 2026 03:33

Copilot started reviewing on behalf of gudaoxuri April 18, 2026 03:34 View session

Copilot AI reviewed Apr 18, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Review/full workspace 2026 04 18#996

Review/full workspace 2026 04 18#996
gudaoxuri wants to merge 5 commits into
mainfrom
review/full-workspace-2026-04-18

gudaoxuri commented Apr 18, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 18, 2026

Uh oh!

Copilot AI Apr 18, 2026

Uh oh!

Copilot AI Apr 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		// Deployments must set `iam.ldap.bind_password` explicitly; this default is intentionally empty.
		bind_password: String::new(),

-            // Deployments must set `iam.ldap.bind_password` explicitly; this default is intentionally empty.
-            bind_password: String::new(),
+            // Use a non-empty sentinel so an omitted configuration cannot accidentally allow
+            // an admin bind with an empty password. Deployments must override this value via
+            // `iam.ldap.bind_password`.
+            bind_password: "__CONFIGURE_LDAP_BIND_PASSWORD__".to_string(),

Conversation

gudaoxuri commented Apr 18, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 18, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 18, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 18, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants