chore: remove jwt from RequestContext#2118
Merged
ehayes2000 merged 1 commit intomainfrom Mar 25, 2026
Merged
Conversation
Contributor
WalkthroughRemoved the deprecated Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
ehayes2000
changed the title
ehayes2000
force-pushed
the
eric/remove-jwt-from-request-context
branch
5 times, most recently
from
bab3804 to
58c4b85
Compare
ehayes2000
force-pushed
the
eric/remove-jwt-from-request-context
branch
2 times, most recently
from
d568985 to
415c770
Compare
![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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
rust/cloud-storage/document_cognition_service/src/api/context/test.rs (1)
158-161: 🧹 Nitpick | 🔵 TrivialUnused
document_cognition_service_clientconstruction.The
document_cognition_service_clientis still being constructed but is no longer passed toScribeClientafter the refactor. Consider removing this dead code.🧹 Proposed fix to remove unused code
- let document_cognition_service_client = Arc::new(DocumentCognitionServiceClient::new( - "dummy_auth_key".into(), - "http://localhost".into(), - ));Also remove the unused import on line 115:
- use document_cognition_service_client::DocumentCognitionServiceClient;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/document_cognition_service/src/api/context/test.rs` around lines 158 - 161, Remove the now-dead construction of document_cognition_service_client created via DocumentCognitionServiceClient::new since it is no longer passed into ScribeClient; delete the let binding for document_cognition_service_client and also remove the corresponding unused import (the DocumentCognitionServiceClient import) so the test file no longer contains unused code or imports.rust/cloud-storage/memory/src/context.rs (1)
27-35: 🧹 Nitpick | 🔵 TrivialOutdated doc comment references removed environment variables.
The doc comment still lists
DOCUMENT_COGNITION_SERVICE_URLandCOMMS_SERVICE_URLas required env vars, but these are no longer read after the refactor. Consider updating the documentation to reflect the actual requirements.📝 Proposed doc comment update
/// Builds a [`ToolServiceContext`] from environment variables and a database pool. /// /// Required env vars: `INTERNAL_API_SECRET_KEY`, `DOCUMENT_STORAGE_SERVICE_URL`, -/// `SEARCH_SERVICE_URL`, `EMAIL_SERVICE_URL`, `SYNC_SERVICE_URL`, -/// `DOCUMENT_COGNITION_SERVICE_URL`, `STATIC_FILE_SERVICE_URL`, +/// `SEARCH_SERVICE_URL`, `EMAIL_SERVICE_URL`, `SYNC_SERVICE_URL`, `STATIC_FILE_SERVICE_URL`, /// `DOCUMENT_STORAGE_BUCKET`, `DOCX_DOCUMENT_UPLOAD_BUCKET`, /// `DOCUMENT_STORAGE_SERVICE_CLOUDFRONT_DISTRIBUTION_URL`, /// `DOCUMENT_STORAGE_SERVICE_CLOUDFRONT_SIGNER_PUBLIC_KEY_ID`, /// `DOCUMENT_STORAGE_SERVICE_CLOUDFRONT_SIGNER_PRIVATE_KEY_SECRET_NAME`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/memory/src/context.rs` around lines 27 - 35, Update the doc comment above the ToolServiceContext builder in context.rs to remove stale environment variables (remove DOCUMENT_COGNITION_SERVICE_URL and COMMS_SERVICE_URL) and ensure the listed required env vars exactly match what the builder actually reads (e.g., INTERNAL_API_SECRET_KEY, DOCUMENT_STORAGE_SERVICE_URL, SEARCH_SERVICE_URL, EMAIL_SERVICE_URL, SYNC_SERVICE_URL, STATIC_FILE_SERVICE_URL, DOCUMENT_STORAGE_BUCKET, DOCX_DOCUMENT_UPLOAD_BUCKET, DOCUMENT_STORAGE_SERVICE_CLOUDFRONT_DISTRIBUTION_URL, DOCUMENT_STORAGE_SERVICE_CLOUDFRONT_SIGNER_PUBLIC_KEY_ID, DOCUMENT_STORAGE_SERVICE_CLOUDFRONT_SIGNER_PRIVATE_KEY_SECRET_NAME or whatever the current code in the ToolServiceContext::from_env/constructor expects); keep the brief one-line description and the fenced reference to ToolServiceContext so the comment stays accurate and up-to-date with the implementation.rust/cloud-storage/document_cognition_service/src/api/stream/chat_message.rs (1)
347-371: 🧹 Nitpick | 🔵 TrivialRemove unused
jwt_tokenparameter fromstream_and_save_message.The
jwt_tokenparameter (line 348) is never used in the function body. AfterRequestContextwas refactored to exclude thejwtfield, this parameter became dead code. Remove it from the function signature and update the call site at line 276 accordingly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/document_cognition_service/src/api/stream/chat_message.rs` around lines 347 - 371, Remove the unused jwt_token parameter from the stream_and_save_message function signature and all call sites: delete the jwt_token parameter in the function declaration and remove the corresponding argument where stream_and_save_message(...) is invoked; adjust the function's parameter list to match usage (keep user_id, chat_id, message_id, stream_id, model, now, user_message_content, user_message_id, user_message_attachments, durable_stream_id) and update any references to RequestContext or other related structs if they relied on the removed argument so compilation succeeds.rust/cloud-storage/scribe/src/channel/client.rs (1)
149-150:⚠️ Potential issue | 🟡 MinorMisleading prompt text: hardcoded "1000 messages" doesn't match actual limit.
The prompt claims the conversation is "limited to the last 1000 messages," but the actual limit is determined by the
limitparameter (which is typicallyMAX_MESSAGES = 150from callers likeread.rs). This could confuse AI systems or users interpreting the transcript.🔧 Proposed fix
- let prompt = "The following conversation is limited to the last 1000 messages. Ignore all formatting and do not show the user the formatted conversation."; + let limit_desc = limit.map(|l| l.to_string()).unwrap_or_else(|| "all".to_string()); + let prompt = format!("The following conversation is limited to the last {} messages. Ignore all formatting and do not show the user the formatted conversation.", limit_desc); let formatted_messages = format!("{}\n\n{}", prompt, formatted_text);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/scribe/src/channel/client.rs` around lines 149 - 150, The prompt string in client.rs is misleading because it hardcodes "1000 messages" while the actual cap is the function's limit parameter (often MAX_MESSAGES); update the prompt construction so it uses the actual limit variable (e.g., interpolate or format with limit or MAX_MESSAGES) instead of the hardcoded 1000, and ensure the prompt and formatted_messages creation reference that variable (look for prompt, formatted_messages, and the function parameter limit / constant MAX_MESSAGES) so the message accurately reflects the real message limit.rust/cloud-storage/ai_tools/src/read.rs (1)
201-231:⚠️ Potential issue | 🔴 CriticalPass
request_contexttoread_channeland add user authorization checks.The
read_channelmethod queries channel metadata and messages without verifying the calling user has access. Unlikeread_projectwhich receives and usesrequest_context.user_id, theread_channelmethod receives onlyscribeand omits authorization entirely.The database queries called—
get_channelandget_messages—do not filter by user or check channel membership. A user can invoke the ReadThread tool to read any channel and its message history regardless of their access permissions.Add authorization by:
- Passing
request_contexttoread_channel(matching the pattern inread_project)- Passing
user_idthroughChannelClientmethods to the database layer- Adding access control in the SQL queries or client methods, following the pattern in
batch_get_channel_previewwhich checks channel type, user participation, and organization membership🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/ai_tools/src/read.rs` around lines 201 - 231, read_channel currently takes only &ToolScribe and fetches channel metadata/transcript without an auth check; change its signature to accept &RequestContext (e.g., async fn read_channel(&self, scribe: &ToolScribe, request_context: &RequestContext) -> ...), extract the requesting user id (request_context.user_id) and pass it into the ChannelClient calls (replace scribe.channel.get_channel_metadata(id) and scribe.channel.get_channel_transcript(id, ...) with calls that accept a user_id argument), and update the ChannelClient methods / SQL layer to enforce the same access control used by batch_get_channel_preview (check channel type, user participation or org membership) so metadata/transcript queries only return results if the user is authorized; preserve existing error mapping to ToolCallError.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rust/cloud-storage/scribe/src/dcs/client.rs`:
- Around line 30-47: The mapping over chat.messages is overwriting each message
timestamp with chrono::Utc::now(), which loses the original send time; inside
the closure that constructs MessageWithAttachments (in the
chat.messages.into_iter().map(...) block), use the message's original timestamp
field (e.g., m.date or whatever timestamp field exists on the message type) when
populating MessageWithAttachments.date and only fall back to chrono::Utc::now()
if that original timestamp is absent/optional; update MessageWithAttachments
population to preserve the original timestamp instead of stamping at read time.
- Around line 10-17: DcsClient lost caller-scoped authorization causing history
reads to be unprotected; restore caller identity checks by reintroducing caller
context and enforcing access checks before any chat history read. Update
DcsClient (constructor/new) to accept and store a caller identity or scope
(e.g., caller_id or AccessScope) or alternatively change the chat history
methods on DcsClient to take an explicit caller parameter, then call the
repository access check (or a new method like PgChatRepo::verify_access /
verify_thread_owner) before performing reads in methods that fetch by chat_id or
message_ids; ensure the same pattern is applied to all affected methods
referenced in the diff (the DcsClient::new area and the history-read methods at
the noted locations) so thread user_id or resolved access scope is validated and
unauthorized requests are rejected.
In `@rust/cloud-storage/scribe/src/document/project.rs`:
- Around line 62-72: In content (the async method on Project in project.rs) you
currently call get_project_content_v2 with a hard-coded AccessLevel::View;
instead first resolve the caller's real access using the existing project access
lookup (e.g. the function/logic that returns a project_user_access_level for
MacroUserIdStr), return an error if the resolved access is None/NoAccess, and
then pass that resolved AccessLevel into get_project_content_v2 so callers keep
their true Edit/Owner rights (do not hard-code AccessLevel::View). Ensure you
reference the same MacroUserIdStr and ProjectFetcher<ScribeProjectPreview>
return path and bail early on insufficient access before calling
get_project_content_v2.
---
Outside diff comments:
In `@rust/cloud-storage/ai_tools/src/read.rs`:
- Around line 201-231: read_channel currently takes only &ToolScribe and fetches
channel metadata/transcript without an auth check; change its signature to
accept &RequestContext (e.g., async fn read_channel(&self, scribe: &ToolScribe,
request_context: &RequestContext) -> ...), extract the requesting user id
(request_context.user_id) and pass it into the ChannelClient calls (replace
scribe.channel.get_channel_metadata(id) and
scribe.channel.get_channel_transcript(id, ...) with calls that accept a user_id
argument), and update the ChannelClient methods / SQL layer to enforce the same
access control used by batch_get_channel_preview (check channel type, user
participation or org membership) so metadata/transcript queries only return
results if the user is authorized; preserve existing error mapping to
ToolCallError.
In `@rust/cloud-storage/document_cognition_service/src/api/context/test.rs`:
- Around line 158-161: Remove the now-dead construction of
document_cognition_service_client created via
DocumentCognitionServiceClient::new since it is no longer passed into
ScribeClient; delete the let binding for document_cognition_service_client and
also remove the corresponding unused import (the DocumentCognitionServiceClient
import) so the test file no longer contains unused code or imports.
In
`@rust/cloud-storage/document_cognition_service/src/api/stream/chat_message.rs`:
- Around line 347-371: Remove the unused jwt_token parameter from the
stream_and_save_message function signature and all call sites: delete the
jwt_token parameter in the function declaration and remove the corresponding
argument where stream_and_save_message(...) is invoked; adjust the function's
parameter list to match usage (keep user_id, chat_id, message_id, stream_id,
model, now, user_message_content, user_message_id, user_message_attachments,
durable_stream_id) and update any references to RequestContext or other related
structs if they relied on the removed argument so compilation succeeds.
In `@rust/cloud-storage/memory/src/context.rs`:
- Around line 27-35: Update the doc comment above the ToolServiceContext builder
in context.rs to remove stale environment variables (remove
DOCUMENT_COGNITION_SERVICE_URL and COMMS_SERVICE_URL) and ensure the listed
required env vars exactly match what the builder actually reads (e.g.,
INTERNAL_API_SECRET_KEY, DOCUMENT_STORAGE_SERVICE_URL, SEARCH_SERVICE_URL,
EMAIL_SERVICE_URL, SYNC_SERVICE_URL, STATIC_FILE_SERVICE_URL,
DOCUMENT_STORAGE_BUCKET, DOCX_DOCUMENT_UPLOAD_BUCKET,
DOCUMENT_STORAGE_SERVICE_CLOUDFRONT_DISTRIBUTION_URL,
DOCUMENT_STORAGE_SERVICE_CLOUDFRONT_SIGNER_PUBLIC_KEY_ID,
DOCUMENT_STORAGE_SERVICE_CLOUDFRONT_SIGNER_PRIVATE_KEY_SECRET_NAME or whatever
the current code in the ToolServiceContext::from_env/constructor expects); keep
the brief one-line description and the fenced reference to ToolServiceContext so
the comment stays accurate and up-to-date with the implementation.
In `@rust/cloud-storage/scribe/src/channel/client.rs`:
- Around line 149-150: The prompt string in client.rs is misleading because it
hardcodes "1000 messages" while the actual cap is the function's limit parameter
(often MAX_MESSAGES); update the prompt construction so it uses the actual limit
variable (e.g., interpolate or format with limit or MAX_MESSAGES) instead of the
hardcoded 1000, and ensure the prompt and formatted_messages creation reference
that variable (look for prompt, formatted_messages, and the function parameter
limit / constant MAX_MESSAGES) so the message accurately reflects the real
message limit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f8a5ae79-7bd6-4ff2-a01f-8b1b945805a0
⛔ Files ignored due to path filters (1)
rust/cloud-storage/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (27)
rust/cloud-storage/ai/src/tool/tool_loop/cli.rsrust/cloud-storage/ai_tools/src/read.rsrust/cloud-storage/ai_tools/src/rewrite/tool.rsrust/cloud-storage/ai_toolset/src/context.rsrust/cloud-storage/chat/src/outbound/postgres/mod.rsrust/cloud-storage/document_cognition_service/src/api/chats/tool/post.rsrust/cloud-storage/document_cognition_service/src/api/context/test.rsrust/cloud-storage/document_cognition_service/src/api/stream/chat_message.rsrust/cloud-storage/document_cognition_service/src/api/stream/util/chat_message/ai_request.rsrust/cloud-storage/document_cognition_service/src/bin/mcp_server.rsrust/cloud-storage/document_cognition_service/src/main.rsrust/cloud-storage/document_cognition_service/src/mcp_oauth/tool_service.rsrust/cloud-storage/document_cognition_service/src/service/attachment/fetch.rsrust/cloud-storage/document_cognition_service/src/service/get_chat.rsrust/cloud-storage/documents/examples/documents_toolset.rsrust/cloud-storage/memory/Cargo.tomlrust/cloud-storage/memory/src/context.rsrust/cloud-storage/memory/src/domain/service.rsrust/cloud-storage/scribe/Cargo.tomlrust/cloud-storage/scribe/src/channel/client.rsrust/cloud-storage/scribe/src/channel/mod.rsrust/cloud-storage/scribe/src/dcs/client.rsrust/cloud-storage/scribe/src/dcs/mod.rsrust/cloud-storage/scribe/src/document/client.rsrust/cloud-storage/scribe/src/document/project.rsrust/cloud-storage/scribe/src/email/client.rsrust/cloud-storage/soup/examples/ai_tools_cli.rs
💤 Files with no reviewable changes (8)
- rust/cloud-storage/documents/examples/documents_toolset.rs
- rust/cloud-storage/document_cognition_service/src/mcp_oauth/tool_service.rs
- rust/cloud-storage/soup/examples/ai_tools_cli.rs
- rust/cloud-storage/memory/src/domain/service.rs
- rust/cloud-storage/ai/src/tool/tool_loop/cli.rs
- rust/cloud-storage/memory/Cargo.toml
- rust/cloud-storage/document_cognition_service/src/api/chats/tool/post.rs
- rust/cloud-storage/ai_toolset/src/context.rs
Comment on lines
+30
to
+47
| let messages = chat | ||
| .messages | ||
| .into_iter() | ||
| .map(|m| { | ||
| let content = match m.content { | ||
| ai::types::ChatMessageContent::Text(s) => s, | ||
| other => serde_json::to_string(&other).unwrap_or_default(), | ||
| }; | ||
| MessageWithAttachments { | ||
| content, | ||
| date: chrono::Utc::now(), | ||
| attachment_ids: m | ||
| .attachments | ||
| .iter() | ||
| .map(|a| a.attachment_id.clone()) | ||
| .collect(), | ||
| } | ||
| }) |
Contributor
There was a problem hiding this comment.
Don't rewrite message timestamps at read time.
Line 40 stamps every message with chrono::Utc::now(), so the returned history no longer reflects when those messages were actually sent. Downstream consumers will see the entire thread as happening at fetch time, which breaks chronology and any recency-based behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/cloud-storage/scribe/src/dcs/client.rs` around lines 30 - 47, The
mapping over chat.messages is overwriting each message timestamp with
chrono::Utc::now(), which loses the original send time; inside the closure that
constructs MessageWithAttachments (in the chat.messages.into_iter().map(...)
block), use the message's original timestamp field (e.g., m.date or whatever
timestamp field exists on the message type) when populating
MessageWithAttachments.date and only fall back to chrono::Utc::now() if that
original timestamp is absent/optional; update MessageWithAttachments population
to preserve the original timestamp instead of stamping at read time.
Comment on lines
+62
to
+72
| pub async fn content( | ||
| &self, | ||
| db: &Pool<Postgres>, | ||
| user_id: MacroUserIdStr<'_>, | ||
| ) -> Result<ProjectFetcher<ScribeProjectPreview>> { | ||
| let items = macro_db_client::projects::get_project::get_project_content_v2( | ||
| db, | ||
| &self.id, | ||
| user_id, | ||
| AccessLevel::View, | ||
| ) |
Contributor
There was a problem hiding this comment.
Look up the caller's real project access before calling get_project_content_v2.
get_project_content_v2 trusts the project_user_access_level argument; it does not authorize or filter the project contents itself. Hard-coding AccessLevel::View here means any caller that knows a project id can read its children, and real Edit/Owner permissions are downgraded to View in the preview. Resolve the caller's actual project access first, bail if they have none, and pass that level through instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/cloud-storage/scribe/src/document/project.rs` around lines 62 - 72, In
content (the async method on Project in project.rs) you currently call
get_project_content_v2 with a hard-coded AccessLevel::View; instead first
resolve the caller's real access using the existing project access lookup (e.g.
the function/logic that returns a project_user_access_level for MacroUserIdStr),
return an error if the resolved access is None/NoAccess, and then pass that
resolved AccessLevel into get_project_content_v2 so callers keep their true
Edit/Owner rights (do not hard-code AccessLevel::View). Ensure you reference the
same MacroUserIdStr and ProjectFetcher<ScribeProjectPreview> return path and
bail early on insufficient access before calling get_project_content_v2.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rust/cloud-storage/memory/src/context.rs (1)
31-37:⚠️ Potential issue | 🟡 MinorUpdate the env-var docs for the new DCS wiring.
build_tool_service_contextno longer readsDOCUMENT_COGNITION_SERVICE_URL, so the doc comment now overstates the required configuration. Please drop it from the list to keep setup docs accurate.Also applies to: 88-89
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/memory/src/context.rs` around lines 31 - 37, The doc comment listing required env vars is outdated: remove DOCUMENT_COGNITION_SERVICE_URL from the comment for build_tool_service_context and the duplicate mention later (the other doc comment around the same file where the list repeats at the referenced lines) so the documented required configuration matches the current code; locate the comment blocks that enumerate env vars and delete the DOCUMENT_COGNITION_SERVICE_URL token from those lists (references: build_tool_service_context).
♻️ Duplicate comments (3)
rust/cloud-storage/scribe/src/document/project.rs (1)
62-72:⚠️ Potential issue | 🔴 CriticalResolve the caller's actual project access before fetching content.
get_project_content_v2trusts theproject_user_access_levelargument; it does not authorize or filter the project first. Hard-codingAccessLevel::Viewhere means callers with no access can still see the children, and realEditorOwnercallers are downcast in the preview. Look up the user's actual project access, bail if none, and pass that level through.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/scribe/src/document/project.rs` around lines 62 - 72, In the content method, don't hard-code AccessLevel::View when calling macro_db_client::projects::get_project::get_project_content_v2; first resolve the caller's actual project access (e.g. call the project's access lookup helper to get the user's AccessLevel for self.id), return an error if the user has no access, then pass that resolved AccessLevel into get_project_content_v2 so callers with Edit/Owner keep their level and unauthorized callers are rejected.rust/cloud-storage/scribe/src/dcs/client.rs (2)
30-47:⚠️ Potential issue | 🟠 MajorPreserve the stored message timestamp.
Line 40 rewrites every message date to fetch time, so the returned history no longer reflects when messages were actually sent. Populate
MessageWithAttachments.datefrom the persisted timestamp onm, and only fall back toUtc::now()if that field is genuinely absent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/scribe/src/dcs/client.rs` around lines 30 - 47, The mapping currently sets MessageWithAttachments.date to chrono::Utc::now(), losing the original send time; change the mapping in the chat.messages iterator so MessageWithAttachments.date is populated from the persisted timestamp on m (e.g., m.date or m.timestamp) and only use chrono::Utc::now() as a fallback when that field is None/missing—update the closure that builds MessageWithAttachments (referencing chat.messages, m, and MessageWithAttachments) to prefer the stored timestamp and fall back to Utc::now().
10-17:⚠️ Potential issue | 🔴 CriticalRestore caller-scoped authorization before these DB reads.
DcsClientnow carries only DB/repo handles, and both history methods are keyed solely bychat_idormessage_ids. With no caller/user scope in the struct or method signatures, this path cannot reject cross-user reads before hitting Postgres.Thread
user_idor an access scope through these methods and verify access before callingPgChatRepo::get_chat(...)/macro_db_client::chat_history::get_chat_history_for_messages(...).Also applies to: 23-28, 61-65
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/scribe/src/dcs/client.rs` around lines 10 - 17, DcsClient currently holds only DB/repo handles so its history methods can perform cross-user reads; modify the API to accept a caller context (e.g., a user_id or AccessScope) on construction or as an explicit parameter to the history methods and enforce authorization before any DB call: check caller scope/ownership and reject unauthorized callers prior to invoking PgChatRepo::get_chat(...) and macro_db_client::chat_history::get_chat_history_for_messages(...). Update DcsClient::new usage sites or the affected methods (the ones that call get_chat and get_chat_history_for_messages) to propagate the caller identity and perform the authorization check early. Ensure tests and call sites are adjusted to supply the caller context.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rust/cloud-storage/ai_tools/src/read.rs`:
- Around line 147-153: The non-project branches (read_channel,
read_channel_message, read_chat_thread, read_chat_messages, read_email_thread,
read_email_message) currently drop the RequestContext, losing caller identity
used for authorization; restore passing the RequestContext (or at least
request_context.user_id) into each of these read_* calls so DB-side or pre-check
authorization can enforce per-user access just like read_project does (update
signatures or call-sites as needed to accept
RequestContext/request_context.user_id and propagate it through the read_*
implementations).
In `@rust/cloud-storage/ai_tools/src/rewrite/tool.rs`:
- Around line 42-47: The current rewrite path drops the caller context and calls
the internal fetch path (keyed only by markdown_file_id), enabling unauthorized
reads; restore and thread the caller's user identity into the fetch so the
user-authorized DB/service branch is used (rather than the no-JWT internal
branch that calls get_recent_document_location_v3). Concretely, in the
rewrite_markdown call flow in tool.rs (and the adjacent fetch usage around lines
referenced), propagate the user_id/caller context into the fetch invocation (or
call the user-scoped fetch variant) so the fetch function enforces per-user
authorization instead of using the bare internal fetch path. Ensure any
ToolCallError mapping remains but do not drop or replace the user context when
calling fetch.
In
`@rust/cloud-storage/document_cognition_service/src/api/stream/chat_message.rs`:
- Around line 249-250: The stream_and_save_message function is currently
capturing the raw jwt_token in spans; update its tracing/instrumentation to
avoid recording sensitive tokens by either removing the jwt_token parameter
entirely (and update all callers that pass it) or add tracing skip for that
parameter (e.g., ensure the #[tracing::instrument] on stream_and_save_message
lists jwt_token in skip(...)); apply the same change to the other affected
function/call site referenced around lines 342-349 so no bearer token is
attached to telemetry/logs.
In
`@rust/cloud-storage/document_cognition_service/src/service/attachment/fetch.rs`:
- Around line 31-37: The channel and email attachment branches currently call
the document fetch/content pipeline without the caller-scoped user_id, allowing
private transcripts to be loaded by ID; update the AttachmentType::Channel and
AttachmentType::Email branches to mirror the project branch by calling
.content(scribe.document.db(), user_id) after their fetch (e.g., where
fetch_channel(...) and fetch_email(...) are used) so the permission check is
applied; apply the same change at the other occurrences noted (around the blocks
corresponding to lines 63-67 and 99-103).
In `@rust/cloud-storage/scribe/src/channel/client.rs`:
- Around line 21-31: The get_channel_metadata path currently fetches channel
info by UUID without any caller identity check; restore per-user authorization
by accepting the caller identity (e.g., a user_id or JWT claims) into
get_channel_metadata and similar methods, validate/convert that identity (like
channel_id .try_into() is done) and perform an access check (call the existing
comms_db_client permission/membership check such as
comms_db_client::channels::is_member or an equivalent ACL method) before
returning ChannelMetadata; if the check fails return an unauthorized Error (do
the same for the other affected methods referenced at 50-62 and 67-85 so every
channel/message read path enforces the same per-user access check).
In `@rust/cloud-storage/scribe/src/email/client.rs`:
- Around line 20-22: The public methods (e.g., get_email_message_by_id and the
other methods at lines 26-35) are calling internal-only lookups
(get_message_by_id_internal / *_internal) and thus drop caller identity; replace
those calls with the user-authorized DB/service paths that enforce per-caller
authorization (e.g., call get_message_by_id or a user-scoped variant that
accepts the caller identity/context) and preserve/forward the caller principal
from the public method (self or request context) into the backend call instead
of using the internal_* helpers; do the same fix for the other listed methods so
authorization is enforced and no unconditional internal fetch is used.
---
Outside diff comments:
In `@rust/cloud-storage/memory/src/context.rs`:
- Around line 31-37: The doc comment listing required env vars is outdated:
remove DOCUMENT_COGNITION_SERVICE_URL from the comment for
build_tool_service_context and the duplicate mention later (the other doc
comment around the same file where the list repeats at the referenced lines) so
the documented required configuration matches the current code; locate the
comment blocks that enumerate env vars and delete the
DOCUMENT_COGNITION_SERVICE_URL token from those lists (references:
build_tool_service_context).
---
Duplicate comments:
In `@rust/cloud-storage/scribe/src/dcs/client.rs`:
- Around line 30-47: The mapping currently sets MessageWithAttachments.date to
chrono::Utc::now(), losing the original send time; change the mapping in the
chat.messages iterator so MessageWithAttachments.date is populated from the
persisted timestamp on m (e.g., m.date or m.timestamp) and only use
chrono::Utc::now() as a fallback when that field is None/missing—update the
closure that builds MessageWithAttachments (referencing chat.messages, m, and
MessageWithAttachments) to prefer the stored timestamp and fall back to
Utc::now().
- Around line 10-17: DcsClient currently holds only DB/repo handles so its
history methods can perform cross-user reads; modify the API to accept a caller
context (e.g., a user_id or AccessScope) on construction or as an explicit
parameter to the history methods and enforce authorization before any DB call:
check caller scope/ownership and reject unauthorized callers prior to invoking
PgChatRepo::get_chat(...) and
macro_db_client::chat_history::get_chat_history_for_messages(...). Update
DcsClient::new usage sites or the affected methods (the ones that call get_chat
and get_chat_history_for_messages) to propagate the caller identity and perform
the authorization check early. Ensure tests and call sites are adjusted to
supply the caller context.
In `@rust/cloud-storage/scribe/src/document/project.rs`:
- Around line 62-72: In the content method, don't hard-code AccessLevel::View
when calling macro_db_client::projects::get_project::get_project_content_v2;
first resolve the caller's actual project access (e.g. call the project's access
lookup helper to get the user's AccessLevel for self.id), return an error if the
user has no access, then pass that resolved AccessLevel into
get_project_content_v2 so callers with Edit/Owner keep their level and
unauthorized callers are rejected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d3698d26-d2b7-447e-86fb-5a0620eac65b
⛔ Files ignored due to path filters (1)
rust/cloud-storage/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (27)
rust/cloud-storage/ai/src/tool/tool_loop/cli.rsrust/cloud-storage/ai_tools/src/read.rsrust/cloud-storage/ai_tools/src/rewrite/tool.rsrust/cloud-storage/ai_toolset/src/context.rsrust/cloud-storage/chat/src/outbound/postgres/mod.rsrust/cloud-storage/document_cognition_service/src/api/chats/tool/post.rsrust/cloud-storage/document_cognition_service/src/api/context/test.rsrust/cloud-storage/document_cognition_service/src/api/stream/chat_message.rsrust/cloud-storage/document_cognition_service/src/api/stream/util/chat_message/ai_request.rsrust/cloud-storage/document_cognition_service/src/bin/mcp_server.rsrust/cloud-storage/document_cognition_service/src/main.rsrust/cloud-storage/document_cognition_service/src/mcp_oauth/tool_service.rsrust/cloud-storage/document_cognition_service/src/service/attachment/fetch.rsrust/cloud-storage/document_cognition_service/src/service/get_chat.rsrust/cloud-storage/documents/examples/documents_toolset.rsrust/cloud-storage/memory/Cargo.tomlrust/cloud-storage/memory/src/context.rsrust/cloud-storage/memory/src/domain/service.rsrust/cloud-storage/scribe/Cargo.tomlrust/cloud-storage/scribe/src/channel/client.rsrust/cloud-storage/scribe/src/channel/mod.rsrust/cloud-storage/scribe/src/dcs/client.rsrust/cloud-storage/scribe/src/dcs/mod.rsrust/cloud-storage/scribe/src/document/client.rsrust/cloud-storage/scribe/src/document/project.rsrust/cloud-storage/scribe/src/email/client.rsrust/cloud-storage/soup/examples/ai_tools_cli.rs
💤 Files with no reviewable changes (5)
- rust/cloud-storage/memory/src/domain/service.rs
- rust/cloud-storage/ai_toolset/src/context.rs
- rust/cloud-storage/memory/Cargo.toml
- rust/cloud-storage/document_cognition_service/src/api/chats/tool/post.rs
- rust/cloud-storage/ai/src/tool/tool_loop/cli.rs
Comment on lines
+147
to
153
| ContentType::Channel => self.read_channel(&scribe).await?, | ||
| ContentType::ChannelMessage => self.read_channel_message(&scribe).await?, | ||
| ContentType::ChatThread => self.read_chat_thread(&scribe).await?, | ||
| ContentType::ChatMessage => self.read_chat_messages(&scribe).await?, | ||
| ContentType::EmailThread => self.read_email_thread(&scribe).await?, | ||
| ContentType::EmailMessage => self.read_email_message(&scribe).await?, | ||
| ContentType::Project => self.read_project(&scribe, &request_context).await?, |
Contributor
There was a problem hiding this comment.
Keep caller identity on the non-project read paths.
Lines 147-152 drop RequestContext from every non-project branch, while the project path below now explicitly passes request_context.user_id for DB-side authorization. After this refactor, the tool no longer has a way to enforce per-user access on channel/chat/email reads before handing them off, which turns these branches into ID-only lookup surfaces.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/cloud-storage/ai_tools/src/read.rs` around lines 147 - 153, The
non-project branches (read_channel, read_channel_message, read_chat_thread,
read_chat_messages, read_email_thread, read_email_message) currently drop the
RequestContext, losing caller identity used for authorization; restore passing
the RequestContext (or at least request_context.user_id) into each of these
read_* calls so DB-side or pre-check authorization can enforce per-user access
just like read_project does (update signatures or call-sites as needed to accept
RequestContext/request_context.user_id and propagate it through the read_*
implementations).
Comment on lines
+42
to
+47
| rewrite_markdown(self.clone(), &scribe) | ||
| .await | ||
| .map_err(|err| ToolCallError { | ||
| description: "An internal error occured rewriting generating rewrite".into(), | ||
| internal_error: err, | ||
| }) |
Contributor
There was a problem hiding this comment.
Restore user-scoped authorization on markdown fetch.
Dropping the caller context here and switching to fetch(...) below leaves this read path keyed only by markdown_file_id. The no-JWT branch in rust/cloud-storage/scribe/src/document/fetcher.rs:62-77 calls get_recent_document_location_v3(&document_id) with no user identity, so this tool can now read or rewrite any markdown document whose id is known. Thread user_id into a user-authorized DB/service fetch instead of the bare internal fetch path.
Also applies to: 57-59
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/cloud-storage/ai_tools/src/rewrite/tool.rs` around lines 42 - 47, The
current rewrite path drops the caller context and calls the internal fetch path
(keyed only by markdown_file_id), enabling unauthorized reads; restore and
thread the caller's user identity into the fetch so the user-authorized
DB/service branch is used (rather than the no-JWT internal branch that calls
get_recent_document_location_v3). Concretely, in the rewrite_markdown call flow
in tool.rs (and the adjacent fetch usage around lines referenced), propagate the
user_id/caller context into the fetch invocation (or call the user-scoped fetch
variant) so the fetch function enforces per-user authorization instead of using
the bare internal fetch path. Ensure any ToolCallError mapping remains but do
not drop or replace the user context when calling fetch.
Comment on lines
+249
to
250
| (*user_id).clone(), | ||
| ) |
Contributor
There was a problem hiding this comment.
Don't record the raw bearer token on this span.
After the JWT field was removed from RequestContext, stream_and_save_message still accepts jwt_token, and #[tracing::instrument] will capture every non-skipped parameter. That attaches the raw access token to telemetry/logs for each streamed message.
At minimum, skip jwt_token here; ideally, remove the parameter entirely if this path no longer needs it.
Minimal fix
-#[tracing::instrument(skip(ctx, request, toolset, user_message_content))]
+#[tracing::instrument(skip(ctx, request, toolset, user_message_content, jwt_token))]Also applies to: 342-349
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/cloud-storage/document_cognition_service/src/api/stream/chat_message.rs`
around lines 249 - 250, The stream_and_save_message function is currently
capturing the raw jwt_token in spans; update its tracing/instrumentation to
avoid recording sensitive tokens by either removing the jwt_token parameter
entirely (and update all callers that pass it) or add tracing skip for that
parameter (e.g., ensure the #[tracing::instrument] on stream_and_save_message
lists jwt_token in skip(...)); apply the same change to the other affected
function/call site referenced around lines 342-349 so no bearer token is
attached to telemetry/logs.
Comment on lines
31
to
37
| AttachmentType::Project => { | ||
| // fetch id's of stuff in folder | ||
| let project_items = scribe | ||
| .document | ||
| .fetch_project(attachment.attachment_id.clone(), jwt.to_owned()) | ||
| .content() | ||
| .fetch_project(attachment.attachment_id.clone()) | ||
| .content(scribe.document.db(), user_id) | ||
| .await? |
Contributor
There was a problem hiding this comment.
Apply caller-scoped auth to channel/email attachments too.
Only the project branch was moved onto content(..., user_id). The channel and email branches still load content from only an attachment ID, so a private transcript or thread can be injected into model context as long as the ID is known. These fetches need the same user_id/permission check that project attachments now have.
Also applies to: 63-67, 99-103
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@rust/cloud-storage/document_cognition_service/src/service/attachment/fetch.rs`
around lines 31 - 37, The channel and email attachment branches currently call
the document fetch/content pipeline without the caller-scoped user_id, allowing
private transcripts to be loaded by ID; update the AttachmentType::Channel and
AttachmentType::Email branches to mirror the project branch by calling
.content(scribe.document.db(), user_id) after their fetch (e.g., where
fetch_channel(...) and fetch_email(...) are used) so the permission check is
applied; apply the same change at the other occurrences noted (around the blocks
corresponding to lines 63-67 and 99-103).
Comment on lines
+21
to
+31
| #[tracing::instrument(skip(self), err)] | ||
| pub async fn get_channel_metadata( | ||
| &self, | ||
| channel_id: impl TryInto<Uuid> + Debug, | ||
| jwt_token: Option<&str>, | ||
| ) -> Result<ChannelMetadata, Error> { | ||
| let channel_id = channel_id | ||
| .try_into() | ||
| .map_err(|_| anyhow::anyhow!("invalid uuid"))?; | ||
|
|
||
| let channel = match jwt_token { | ||
| Some(token) => self | ||
| .inner | ||
| .get_channel_metadata_external(&channel_id, token) | ||
| .await | ||
| .map_err(Error::from)?, | ||
| None => { | ||
| let channel = | ||
| comms_db_client::channels::get_channel::get_channel(&self.db, &channel_id) | ||
| .await?; | ||
|
|
||
| ChannelMetadataResponse { | ||
| channel_name: channel.name.unwrap_or_default(), | ||
| channel_type: channel.channel_type, | ||
| } | ||
| } | ||
| }; | ||
| let channel = | ||
| comms_db_client::channels::get_channel::get_channel(&self.db, &channel_id).await?; |
Contributor
There was a problem hiding this comment.
Restore per-user authorization on channel reads.
These methods now execute raw comms_db_client lookups from only a channel/message UUID. With the old JWT parameter removed and no user_id or access check replacing it, callers can read private channels and DMs by ID alone. Thread caller identity into this API and reject unauthorized reads before returning metadata or formatted content.
Also applies to: 50-62, 67-85
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/cloud-storage/scribe/src/channel/client.rs` around lines 21 - 31, The
get_channel_metadata path currently fetches channel info by UUID without any
caller identity check; restore per-user authorization by accepting the caller
identity (e.g., a user_id or JWT claims) into get_channel_metadata and similar
methods, validate/convert that identity (like channel_id .try_into() is done)
and perform an access check (call the existing comms_db_client
permission/membership check such as comms_db_client::channels::is_member or an
equivalent ACL method) before returning ChannelMetadata; if the check fails
return an unauthorized Error (do the same for the other affected methods
referenced at 50-62 and 67-85 so every channel/message read path enforces the
same per-user access check).
Comment on lines
+20
to
+22
| #[tracing::instrument(skip(self), err)] | ||
| pub async fn get_email_message_by_id(&self, message_id: &str) -> Result<ParsedMessage> { | ||
| self.inner.get_message_by_id_internal(message_id).await |
Contributor
There was a problem hiding this comment.
Don't replace user-authenticated email reads with bare internal ID lookups.
These methods now drop the caller identity and always hit *_internal(...) using only message_id or thread_id. That removes the information needed for a caller-specific authorization check, so any internal path that learns an id can read mail outside the current user. Keep a user-authorized DB/service path here instead of unconditional internal fetches.
Also applies to: 26-35
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/cloud-storage/scribe/src/email/client.rs` around lines 20 - 22, The
public methods (e.g., get_email_message_by_id and the other methods at lines
26-35) are calling internal-only lookups (get_message_by_id_internal /
*_internal) and thus drop caller identity; replace those calls with the
user-authorized DB/service paths that enforce per-caller authorization (e.g.,
call get_message_by_id or a user-scoped variant that accepts the caller
identity/context) and preserve/forward the caller principal from the public
method (self or request context) into the backend call instead of using the
internal_* helpers; do the same fix for the other listed methods so
authorization is enforced and no unconditional internal fetch is used.
Remove the deprecated jwt field from RequestContext by refactoring all tools and scribe clients to use direct DB access instead of JWT-authenticated HTTP calls. Scribe changes: - DcsClient: uses chat hex crate (PgChatRepo) for get_chat_history, macro_db_client for get_chat_history_for_messages - ChannelClient: removed CommsServiceClient dependency entirely, all methods use direct comms_db_client queries. Removed list_channels (dead code), removed JWT Option params from metadata/transcript. - EmailClient: removed JWT Option params and external HTTP branches, all methods use internal service endpoints only. Removed batch method (dead code). - DocumentClient: fetch_project uses macro_db_client directly - Removed comms_service_client and document_cognition_service_client dependencies from scribe Tool changes: - ReadThread: all methods use internal (non-JWT) scribe paths - MarkdownRewrite: uses fetch() instead of fetch_with_auth() - ai_toolset RequestContext: removed jwt field DCS changes: - Removed unused DocumentCognitionServiceClient from scribe wiring - Threaded user_id through fetchium for project fetching - Updated all callers for simplified scribe APIs
ehayes2000
force-pushed
the
eric/remove-jwt-from-request-context
branch
from
415c770 to
1696d23
Compare
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
rust/cloud-storage/document_cognition_service/src/main.rs (1)
98-98:⚠️ Potential issue | 🔴 CriticalRemove unused
comms_service_clientfield fromApiContextand its initialization.Search of the codebase confirms
comms_service_clientis never accessed anywhere. The field is defined inApiContext(api/context/mod.rs:38), constructed in main.rs line 98, but has zero usages in production code. This is dead code that should be removed from the struct definition and its initialization in main.rs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/document_cognition_service/src/main.rs` at line 98, Remove the unused comms_service_client field by deleting its declaration from the ApiContext struct (symbol: comms_service_client) and removing its initialization where CommsServiceClient::new(...) is called in main (symbol: CommsServiceClient::new and the local variable comms_service_client). Update any ApiContext construction sites (e.g., where ApiContext::new or struct literal is created) to stop supplying comms_service_client and run cargo build to ensure no remaining references exist; if any tests or mocks reference comms_service_client, remove or adapt them accordingly.rust/cloud-storage/document_cognition_service/src/api/stream/util/chat_message/ai_request.rs (1)
14-23:⚠️ Potential issue | 🟡 MinorAdd explicit
user_idfield specification to match codebase patterns.The
user_idparameter will be auto-captured bytracing::instrumentand logged via its Debug impl. The codebase pattern is to explicitly specify user IDs in thefields()attribute rather than skipping them (see similar patterns inchat_history_batch_messages.rs,search_service, andproperties_service).🔧 Recommended fix
-#[tracing::instrument(skip(ctx, chat, incoming_message, static_system_prompt, jwt), err)] +#[tracing::instrument( + skip(ctx, chat, incoming_message, static_system_prompt, jwt), + fields(user_id = %user_id), + err +)]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/document_cognition_service/src/api/stream/util/chat_message/ai_request.rs` around lines 14 - 23, Update the tracing attribute on build_chat_completion_request to explicitly include the user_id field in fields(), e.g. change #[tracing::instrument(skip(ctx, chat, incoming_message, static_system_prompt, jwt), err)] to include fields(user_id = ?user_id) (or use %user_id if Display is preferred) so the user id is logged consistently with the project's pattern; ensure the skip list still contains the same parameters.
♻️ Duplicate comments (8)
rust/cloud-storage/scribe/src/document/project.rs (1)
62-79:⚠️ Potential issue | 🔴 CriticalAuthorization bypass: hard-coded
AccessLevel::Viewskips actual permission verification.The past review correctly identified this issue. Per the context snippet,
get_project_content_v2trusts the passedproject_user_access_levelwithout verifying the user's actual access. Hard-codingAccessLevel::Viewmeans:
- Any user with any project ID can read its contents
- Users with
Edit/Ownerpermissions are incorrectly downgraded toViewin the responseResolve the caller's actual project access level first (bail if none), then pass that level through.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/scribe/src/document/project.rs` around lines 62 - 79, The content method in ProjectFetcher currently hard-codes AccessLevel::View when calling macro_db_client::projects::get_project::get_project_content_v2, causing authorization bypass and downgrading higher-permission users; change it to first resolve the caller's actual access level (e.g., call the DB/ACL helper to fetch the user's project access for self.id and user_id, return an error if no access), then pass that resolved AccessLevel into get_project_content_v2 so the returned ScribeProjectPreview and ProjectFetcher respect the real permissions.rust/cloud-storage/document_cognition_service/src/api/stream/chat_message.rs (1)
342-348:⚠️ Potential issue | 🟠 MajorSensitive token logged:
jwt_tokenis captured in tracing spans.The
#[tracing::instrument]at line 342 skipsctx,request,toolset, anduser_message_content, butjwt_token(line 348) is still captured. This attaches the raw bearer token to telemetry/logs for every streamed message.Add
jwt_tokento the skip list:-#[tracing::instrument(skip(ctx, request, toolset, user_message_content))] +#[tracing::instrument(skip(ctx, request, toolset, user_message_content, jwt_token))]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/document_cognition_service/src/api/stream/chat_message.rs` around lines 342 - 348, The tracing span for stream_and_save_message currently captures the raw jwt_token (sensitive) because it isn't skipped; update the #[tracing::instrument(...)] attribute on stream_and_save_message to include jwt_token in the skip list so the bearer token is not attached to spans/logs, ensuring the attribute lists ctx, request, toolset, user_message_content, and jwt_token.rust/cloud-storage/scribe/src/email/client.rs (1)
20-35:⚠️ Potential issue | 🔴 CriticalAuthorization bypass: internal lookups skip caller permission checks.
The past review correctly flagged this concern. Both methods now unconditionally call
*_internalvariants, which bypass user authorization. Any caller knowing amessage_idorthread_idcan read emails regardless of their actual access rights.Either:
- Restore user-authorized paths that verify the caller has access to the email/thread, or
- Ensure all callers of
EmailClienthave already performed authorization checks upstream (and document this contract).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/scribe/src/email/client.rs` around lines 20 - 35, The methods get_email_message_by_id and get_email_messages_by_thread_id currently call the internal helpers (get_message_by_id_internal and get_messages_by_thread_id_internal) which bypass authorization; update these methods to perform or delegate an explicit authorization check before returning data — either call the public/non-internal variants that enforce access control or add a permission check (e.g., verify caller identity/ownership/scopes) and only call the *_internal functions after authorization passes; alternatively, if you decide to keep internal calls, document and enforce that all EmailClient callers must authorize upstream and add assertions/logging in get_email_message_by_id and get_email_messages_by_thread_id to guarantee the contract.rust/cloud-storage/ai_tools/src/read.rs (1)
146-152:⚠️ Potential issue | 🔴 CriticalPropagate caller identity through the non-project read paths.
read_projectstill passesrequest_context.user_idinto the DB-backed fetch, but the channel/chat/email branches now dropRequestContextentirely. With the Scribe clients now reading from Postgres directly, these helpers become ID-only lookups unless you threaduser_idback through the helper signatures and underlying Scribe calls.Also applies to: 201-376
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/ai_tools/src/read.rs` around lines 146 - 152, The non-project branches of the match (ContentType::Channel/ChannelMessage/ChatThread/ChatMessage/EmailThread/EmailMessage) drop RequestContext and thus lose caller identity; update the helper signatures (read_channel, read_channel_message, read_chat_thread, read_chat_messages, read_email_thread, read_email_message) to accept the caller user_id (or RequestContext) and propagate that value into the Scribe client calls and any DB-backed fetches they perform, mirroring how read_project passes request_context.user_id, and adjust all call sites in read.rs (and the underlying scribe methods) so the DB lookups become identity-aware rather than ID-only.rust/cloud-storage/document_cognition_service/src/service/attachment/fetch.rs (1)
16-21:⚠️ Potential issue | 🔴 CriticalApply the new
user_idto every attachment read, not just projects.This change threads
user_idthroughfetchium, but only the project branch on Lines 35-36 consumes it. Channel and email attachments are still fetched by ID alone, so the caller-scoped protection added here remains incomplete for private transcripts and threads. Threaduser_idinto those client APIs as well.Also applies to: 24-29, 31-37, 139-145
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/document_cognition_service/src/service/attachment/fetch.rs` around lines 16 - 21, fetchium now accepts user_id but only applies it for project attachments; update the channel and email attachment fetch paths to pass the same user_id to their client APIs so caller-scoped protection is applied uniformly. Locate the fetchium function and the branches handling ChatAttachmentWithName variants (project, channel, email) and modify the channel- and email-related calls to include the user_id argument (the same MacroUserIdStr<'static> passed into fetchium) when invoking the underlying client methods that load attachments, ensuring all Attachment fetches use the user_id guard like the project branch does.rust/cloud-storage/scribe/src/dcs/client.rs (1)
30-47:⚠️ Potential issue | 🟠 MajorPreserve persisted message timestamps instead of synthesizing them.
Line 40 stamps every entry with
chrono::Utc::now(), soget_chat_history()no longer reflects when messages were actually sent. Theget_chat_history_for_messages()path below already uses a timestamp-preserving query; this method needs the same kind of source/model instead of reconstructing history with fetch-time dates.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/scribe/src/dcs/client.rs` around lines 30 - 47, The mapping currently overwrites original message times by setting MessageWithAttachments.date = chrono::Utc::now(); instead, read and preserve the persisted timestamp from the incoming message struct (use the message's existing timestamp/date field on `m`—e.g., `m.date`/`m.timestamp`/`m.sent_at` as appropriate) and assign that to `MessageWithAttachments.date` so this path mirrors the timestamp-preserving behavior used by `get_chat_history_for_messages()`.rust/cloud-storage/scribe/src/channel/client.rs (1)
15-18:⚠️ Potential issue | 🔴 CriticalRestore caller-scoped authorization before these DB reads.
The constructor now keeps only a raw DB pool, and every public read method here fetches by channel/message UUID alone. That means metadata, transcripts, and surrounding message context for private channels and DMs can be returned without any membership check once an ID is known. Thread caller identity into this client and verify access before calling the DB helpers.
Also applies to: 21-31, 50-62, 67-85
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/scribe/src/channel/client.rs` around lines 15 - 18, The ChannelClient currently only stores a raw DB pool (ChannelClient::new) which allows public read methods to fetch channel/message data by UUID without authorization; change the constructor and struct to accept and store a caller identity/context (e.g., a Caller or UserId) and thread that identity into every public read method (e.g., the methods that fetch channel metadata, messages, message context, transcript) so each method performs an access check against the DB (membership/DM ownership/visibility) before calling the DB helpers; implement and call a helper like ensure_member_or_visible(channel_id, caller) or ensure_caller_can_view_message(message_id, caller) and return an authorization error when the check fails.rust/cloud-storage/ai_tools/src/rewrite/tool.rs (1)
42-47:⚠️ Potential issue | 🔴 CriticalReintroduce caller-scoped document fetch here.
Lines 42-47 drop the caller context, and Line 57 switches to
scribe.document.fetch(...). That path builds a no-token fetcher and falls back to internal DSL reads by document ID, so the rewrite tool can now load any known markdown file without the caller’s authorization context. Use a user-scoped fetch path here instead of the bare internal fetch.Also applies to: 55-59
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/ai_tools/src/rewrite/tool.rs` around lines 42 - 47, The rewrite tool is currently invoking rewrite_markdown(self.clone(), &scribe) which allows scribe.document.fetch to run as a no-token internal fetch; change the flow so the document is loaded with the caller's authorization context instead of the internal fetch. Concretely, ensure you either (A) perform a caller-scoped fetch before calling rewrite_markdown (use the caller-aware variant of scribe.document.fetch or the helper that accepts the caller token) and pass the fetched document or caller-scoped scribe into rewrite_markdown, or (B) extend rewrite_markdown to accept the caller context and use that when invoking scribe.document.fetch; apply the same fix to the other occurrence around lines 55-59 so both paths use caller-scoped fetching rather than the bare internal fetch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@rust/cloud-storage/document_cognition_service/src/api/stream/util/chat_message/ai_request.rs`:
- Around line 14-23: Update the tracing attribute on
build_chat_completion_request to explicitly include the user_id field in
fields(), e.g. change #[tracing::instrument(skip(ctx, chat, incoming_message,
static_system_prompt, jwt), err)] to include fields(user_id = ?user_id) (or use
%user_id if Display is preferred) so the user id is logged consistently with the
project's pattern; ensure the skip list still contains the same parameters.
In `@rust/cloud-storage/document_cognition_service/src/main.rs`:
- Line 98: Remove the unused comms_service_client field by deleting its
declaration from the ApiContext struct (symbol: comms_service_client) and
removing its initialization where CommsServiceClient::new(...) is called in main
(symbol: CommsServiceClient::new and the local variable comms_service_client).
Update any ApiContext construction sites (e.g., where ApiContext::new or struct
literal is created) to stop supplying comms_service_client and run cargo build
to ensure no remaining references exist; if any tests or mocks reference
comms_service_client, remove or adapt them accordingly.
---
Duplicate comments:
In `@rust/cloud-storage/ai_tools/src/read.rs`:
- Around line 146-152: The non-project branches of the match
(ContentType::Channel/ChannelMessage/ChatThread/ChatMessage/EmailThread/EmailMessage)
drop RequestContext and thus lose caller identity; update the helper signatures
(read_channel, read_channel_message, read_chat_thread, read_chat_messages,
read_email_thread, read_email_message) to accept the caller user_id (or
RequestContext) and propagate that value into the Scribe client calls and any
DB-backed fetches they perform, mirroring how read_project passes
request_context.user_id, and adjust all call sites in read.rs (and the
underlying scribe methods) so the DB lookups become identity-aware rather than
ID-only.
In `@rust/cloud-storage/ai_tools/src/rewrite/tool.rs`:
- Around line 42-47: The rewrite tool is currently invoking
rewrite_markdown(self.clone(), &scribe) which allows scribe.document.fetch to
run as a no-token internal fetch; change the flow so the document is loaded with
the caller's authorization context instead of the internal fetch. Concretely,
ensure you either (A) perform a caller-scoped fetch before calling
rewrite_markdown (use the caller-aware variant of scribe.document.fetch or the
helper that accepts the caller token) and pass the fetched document or
caller-scoped scribe into rewrite_markdown, or (B) extend rewrite_markdown to
accept the caller context and use that when invoking scribe.document.fetch;
apply the same fix to the other occurrence around lines 55-59 so both paths use
caller-scoped fetching rather than the bare internal fetch.
In
`@rust/cloud-storage/document_cognition_service/src/api/stream/chat_message.rs`:
- Around line 342-348: The tracing span for stream_and_save_message currently
captures the raw jwt_token (sensitive) because it isn't skipped; update the
#[tracing::instrument(...)] attribute on stream_and_save_message to include
jwt_token in the skip list so the bearer token is not attached to spans/logs,
ensuring the attribute lists ctx, request, toolset, user_message_content, and
jwt_token.
In
`@rust/cloud-storage/document_cognition_service/src/service/attachment/fetch.rs`:
- Around line 16-21: fetchium now accepts user_id but only applies it for
project attachments; update the channel and email attachment fetch paths to pass
the same user_id to their client APIs so caller-scoped protection is applied
uniformly. Locate the fetchium function and the branches handling
ChatAttachmentWithName variants (project, channel, email) and modify the
channel- and email-related calls to include the user_id argument (the same
MacroUserIdStr<'static> passed into fetchium) when invoking the underlying
client methods that load attachments, ensuring all Attachment fetches use the
user_id guard like the project branch does.
In `@rust/cloud-storage/scribe/src/channel/client.rs`:
- Around line 15-18: The ChannelClient currently only stores a raw DB pool
(ChannelClient::new) which allows public read methods to fetch channel/message
data by UUID without authorization; change the constructor and struct to accept
and store a caller identity/context (e.g., a Caller or UserId) and thread that
identity into every public read method (e.g., the methods that fetch channel
metadata, messages, message context, transcript) so each method performs an
access check against the DB (membership/DM ownership/visibility) before calling
the DB helpers; implement and call a helper like
ensure_member_or_visible(channel_id, caller) or
ensure_caller_can_view_message(message_id, caller) and return an authorization
error when the check fails.
In `@rust/cloud-storage/scribe/src/dcs/client.rs`:
- Around line 30-47: The mapping currently overwrites original message times by
setting MessageWithAttachments.date = chrono::Utc::now(); instead, read and
preserve the persisted timestamp from the incoming message struct (use the
message's existing timestamp/date field on `m`—e.g.,
`m.date`/`m.timestamp`/`m.sent_at` as appropriate) and assign that to
`MessageWithAttachments.date` so this path mirrors the timestamp-preserving
behavior used by `get_chat_history_for_messages()`.
In `@rust/cloud-storage/scribe/src/document/project.rs`:
- Around line 62-79: The content method in ProjectFetcher currently hard-codes
AccessLevel::View when calling
macro_db_client::projects::get_project::get_project_content_v2, causing
authorization bypass and downgrading higher-permission users; change it to first
resolve the caller's actual access level (e.g., call the DB/ACL helper to fetch
the user's project access for self.id and user_id, return an error if no
access), then pass that resolved AccessLevel into get_project_content_v2 so the
returned ScribeProjectPreview and ProjectFetcher respect the real permissions.
In `@rust/cloud-storage/scribe/src/email/client.rs`:
- Around line 20-35: The methods get_email_message_by_id and
get_email_messages_by_thread_id currently call the internal helpers
(get_message_by_id_internal and get_messages_by_thread_id_internal) which bypass
authorization; update these methods to perform or delegate an explicit
authorization check before returning data — either call the public/non-internal
variants that enforce access control or add a permission check (e.g., verify
caller identity/ownership/scopes) and only call the *_internal functions after
authorization passes; alternatively, if you decide to keep internal calls,
document and enforce that all EmailClient callers must authorize upstream and
add assertions/logging in get_email_message_by_id and
get_email_messages_by_thread_id to guarantee the contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9bfe70f7-3d1d-43e6-9b54-6e69769361cd
⛔ Files ignored due to path filters (1)
rust/cloud-storage/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (28)
rust/cloud-storage/ai/src/tool/tool_loop/cli.rsrust/cloud-storage/ai_tools/src/read.rsrust/cloud-storage/ai_tools/src/rewrite/tool.rsrust/cloud-storage/ai_toolset/src/context.rsrust/cloud-storage/chat/src/outbound/postgres/mod.rsrust/cloud-storage/document_cognition_service/src/api/chats/tool/post.rsrust/cloud-storage/document_cognition_service/src/api/context/test.rsrust/cloud-storage/document_cognition_service/src/api/stream/chat_message.rsrust/cloud-storage/document_cognition_service/src/api/stream/util/chat_message/ai_request.rsrust/cloud-storage/document_cognition_service/src/bin/mcp_server.rsrust/cloud-storage/document_cognition_service/src/main.rsrust/cloud-storage/document_cognition_service/src/mcp_oauth/tool_service.rsrust/cloud-storage/document_cognition_service/src/service/attachment/fetch.rsrust/cloud-storage/document_cognition_service/src/service/get_chat.rsrust/cloud-storage/documents/examples/documents_toolset.rsrust/cloud-storage/email/examples/ai_tools_cli.rsrust/cloud-storage/memory/Cargo.tomlrust/cloud-storage/memory/src/context.rsrust/cloud-storage/memory/src/domain/service.rsrust/cloud-storage/scribe/Cargo.tomlrust/cloud-storage/scribe/src/channel/client.rsrust/cloud-storage/scribe/src/channel/mod.rsrust/cloud-storage/scribe/src/dcs/client.rsrust/cloud-storage/scribe/src/dcs/mod.rsrust/cloud-storage/scribe/src/document/client.rsrust/cloud-storage/scribe/src/document/project.rsrust/cloud-storage/scribe/src/email/client.rsrust/cloud-storage/soup/examples/ai_tools_cli.rs
💤 Files with no reviewable changes (5)
- rust/cloud-storage/memory/Cargo.toml
- rust/cloud-storage/ai_toolset/src/context.rs
- rust/cloud-storage/ai/src/tool/tool_loop/cli.rs
- rust/cloud-storage/document_cognition_service/src/api/chats/tool/post.rs
- rust/cloud-storage/memory/src/domain/service.rs
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
1 participant
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.
remove jwt from
RequestContext