Feat/keys fresh-implement secure embed authorization#116
Conversation
Yashb404
commented
Feb 11, 2026
Yashb404
commented
- Added feature for private keys for one to one sharing links and domain whitelisting
…hitelisting functionality. Update project handler to ensure VIP keys are generated for project owners and include embed_key in project queries.
There was a problem hiding this comment.
Pull request overview
This pull request implements a secure embed authorization system with dual-layer security: private VIP keys for one-to-one sharing and domain whitelisting for controlled public access. The feature allows project owners to share their terminals privately via unique keys or restrict public embeds to specific authorized domains.
Changes:
- Added
embed_keycolumn to projects table andproject_whiteliststable for domain-based access control - Implemented VIP key authentication and Referer header validation in project access endpoint
- Added three new API endpoints for managing whitelist: GET/POST/DELETE
/api/project/:slug/whitelist - Updated client UI with enhanced Share/Embed modal showing VIP links, smart links, and whitelist management
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 22 comments.
Show a summary per file
| File | Description |
|---|---|
| server/src/models.rs | Added embed_key field to ProjectSummary and WhitelistRequest model for API payload |
| server/src/handlers/project.rs | Implemented dual-layer security (VIP key + whitelist), added three whitelist management endpoints, and lazy VIP key generation |
| server/migrations/20260210000000_secure_embed_schema.up.sql | Created schema for embed_key column and project_whitelists table with appropriate indexes |
| client/src/pages/view.rs | Added VIP link support with query parameters, whitelist resource fetching, and Unauthorized state handling |
| client/src/pages/embed.rs | Added Unauthorized state handling for embed pages with 403 error display |
| client/src/components/modal.rs | Enhanced Share/Embed modal with VIP link section and interactive whitelist management UI |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let referer = headers | ||
| .get("Referer") | ||
| .and_then(|v| v.to_str().ok()) | ||
| .map(|s| s.to_string()); | ||
|
|
||
| // Optional boolean, safely unwrapped later | ||
| let whitelist_allowed: Option<bool> = if let Some(referer_url) = referer { | ||
| let exists_row: Option<(bool,)> = sqlx::query_as( | ||
| "SELECT TRUE FROM project_whitelists \ | ||
| WHERE project_id = $1 AND allowed_url = $2 \ | ||
| LIMIT 1", | ||
| ) | ||
| .bind(project_id) | ||
| .bind(&referer_url) |
There was a problem hiding this comment.
The Referer header validation uses an exact string match without any URL normalization or pattern matching. This creates a security vulnerability because:
- The Referer header is stored and compared as-is, but Referer values can include query parameters, fragments, and trailing slashes which would fail exact matches
- An attacker could easily bypass the whitelist by adding a query parameter (e.g.,
https://medium.com/@user/article?bypass=1would not matchhttps://medium.com/@user/article) - Different paths on the same domain require separate whitelist entries
Consider implementing URL parsing and normalization to compare origins or origin+path without query parameters and fragments. For example, parse the URL and compare just the scheme + host + path components, or provide pattern matching options (e.g., allow wildcards or prefix matching).
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| // 3. Ensure owners always have a VIP key (embed_key); generate one lazily if missing | ||
| if is_owner && embed_key.is_none() { | ||
| let new_key_row: Option<(String,)> = sqlx::query_as( | ||
| "UPDATE projects \ | ||
| SET embed_key = encode(gen_random_bytes(24), 'base64') \ | ||
| WHERE id = $1 \ | ||
| RETURNING embed_key", | ||
| ) | ||
| .bind(project_id) | ||
| .fetch_optional(&state.db) | ||
| .await | ||
| .map_err(|e| { | ||
| ( | ||
| StatusCode::INTERNAL_SERVER_ERROR, | ||
| format!("Failed to generate VIP key: {}", e), | ||
| ) | ||
| })?; | ||
|
|
||
| if let Some((k,)) = new_key_row { | ||
| embed_key = Some(k); | ||
| } | ||
| } |
There was a problem hiding this comment.
The embed_key is generated lazily only when the owner views the project, but it's needed for sharing. This creates an issue where:
- A newly published project won't have an embed_key until the owner views it
- The migration backfills existing projects but new projects created after this won't get a key at creation time
- The owner must view their own project before they can share it with a VIP link
Consider generating the embed_key at project creation time in the publish_handler to ensure it's always available when needed for sharing.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| if let Ok(builder) = req { | ||
| let _ = builder.send().await; | ||
| whitelist_resource.refetch(); | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| let remove_whitelist_item = create_action(move |url: &String| { | ||
| let url = url.clone(); | ||
| let s = slug(); | ||
| async move { | ||
| let req = Request::delete(&format!("{}/api/project/{}/whitelist", api_base(), s)) | ||
| .credentials(RequestCredentials::Include) | ||
| .json(&serde_json::json!({ "allowed_url": url })); | ||
|
|
||
| if let Ok(builder) = req { | ||
| let _ = builder.send().await; | ||
| whitelist_resource.refetch(); | ||
| } |
There was a problem hiding this comment.
The error handling for both add and remove actions silently ignores failures with let _ = builder.send().await;. This means:
- Users won't know if their whitelist modification failed
- The UI will still trigger a refetch even if the operation failed
- Network errors or authorization failures are hidden from the user
Consider handling the response and showing appropriate error messages to the user, similar to how other parts of the application handle errors.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| // Whitelist Resource for Owners | ||
| let whitelist_resource = create_resource( | ||
| move || (project_resource.get(), slug()), | ||
| move |(state, s)| async move { | ||
| if let Some(ProjectState::Ready(_)) = state { | ||
| let url = format!("{}/api/project/{}/whitelist", api_base(), s); | ||
| if let Ok(resp) = Request::get(&url).credentials(RequestCredentials::Include).send().await { | ||
| if let Ok(list) = resp.json::<Vec<String>>().await { | ||
| set_whitelist.set(list); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| ); |
There was a problem hiding this comment.
The whitelist resource is fetched for all owners viewing any project, even if they're viewing someone else's project. The condition checks if project_resource state is Ready but doesn't verify ownership before attempting to fetch the whitelist. This will cause:
- Unnecessary API calls for non-owners (will fail with 404/403 but still wastes resources)
- Console errors that confuse debugging
Consider only fetching the whitelist when is_owner() returns true, or add ownership check to the resource dependency.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| pub async fn add_to_whitelist( | ||
| State(state): State<AppState>, | ||
| session: Session, | ||
| Path(slug): Path<String>, | ||
| Json(payload): Json<WhitelistRequest>, | ||
| ) -> Result<StatusCode, (StatusCode, String)> { | ||
| let user: Option<User> = session | ||
| .get("user") | ||
| .await | ||
| .map_err(|e| (StatusCode::INTERNAL_SERVER_ERROR, format!("Session Error: {}", e)))?; | ||
| let user = user.ok_or((StatusCode::UNAUTHORIZED, "Unauthorized".to_string()))?; | ||
|
|
||
| let trimmed_url = payload.allowed_url.trim(); | ||
| if trimmed_url.is_empty() { | ||
| return Err((StatusCode::BAD_REQUEST, "allowed_url is required".to_string())); | ||
| } | ||
|
|
||
| let project_row: Option<(i64,)> = sqlx::query_as( | ||
| "SELECT id FROM projects WHERE owner_id = $1 AND LOWER(slug) = LOWER($2)", | ||
| ) | ||
| .bind(user.id) | ||
| .bind(&slug) | ||
| .fetch_optional(&state.db) | ||
| .await | ||
| .map_err(|e| (StatusCode::INTERNAL_SERVER_ERROR, format!("DB Error: {}", e)))?; | ||
|
|
||
| let (project_id,) = match project_row { | ||
| Some(row) => row, | ||
| None => return Err((StatusCode::NOT_FOUND, "Project not found".to_string())), | ||
| }; | ||
|
|
||
| // Unique(project_id, allowed_url) is enforced by the DB; ignore conflicts | ||
| let result = sqlx::query( | ||
| "INSERT INTO project_whitelists (project_id, allowed_url) \ | ||
| VALUES ($1, $2) \ | ||
| ON CONFLICT (project_id, allowed_url) DO NOTHING", | ||
| ) | ||
| .bind(project_id) | ||
| .bind(trimmed_url) | ||
| .execute(&state.db) | ||
| .await; | ||
|
|
||
| if let Err(e) = result { | ||
| return Err((StatusCode::INTERNAL_SERVER_ERROR, format!("DB Error: {}", e))); | ||
| } | ||
|
|
||
| Ok(StatusCode::CREATED) | ||
| } |
There was a problem hiding this comment.
There's a potential for Cross-Site Request Forgery (CSRF) attacks on the whitelist management endpoints. The add_to_whitelist and remove_from_whitelist endpoints don't have CSRF protection, which means an attacker could:
- Create a malicious page that sends POST/DELETE requests to these endpoints
- Trick an authenticated owner into visiting the page
- Modify the whitelist without the owner's knowledge
Consider implementing CSRF protection using tokens or SameSite cookie attributes. If using tower-sessions, ensure CSRF protection is enabled for state-modifying operations.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| if let Some(key) = embed_key { | ||
| response_json["embed_key"] = serde_json::Value::String(key); | ||
| } |
There was a problem hiding this comment.
The embed_key is returned to the owner in the JSON response and could be exposed in browser dev tools or network inspection. While this is somewhat acceptable since only owners get it, it increases the risk of accidental exposure if:
- The owner shares their screen while inspecting network traffic
- Browser extensions or debugging tools log the response
- The response is cached insecurely
Consider documenting this security consideration or implementing additional protections like requiring a separate authenticated endpoint to retrieve the embed_key specifically when needed for sharing.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| let row_result = sqlx::query_as::<_, (i64, String, String, String, i64, Option<String>)>( | ||
| "SELECT id, image_tag, markdown, shell, owner_id, embed_key \ | ||
| FROM projects \ | ||
| WHERE LOWER(owner_username) = LOWER($1) AND LOWER(slug) = LOWER($2)" | ||
| ) | ||
| .bind(&username).bind(&slug) | ||
| .fetch_optional(&state.db) | ||
| .await | ||
| .map_err(|e| (StatusCode::INTERNAL_SERVER_ERROR, format!("DB Read Error: {}", e)))?; | ||
|
|
||
| let (image_tag, markdown, shell, owner_id) = match row_result { | ||
| let (project_id, image_tag, markdown, shell, owner_id, mut embed_key) = match row_result { |
There was a problem hiding this comment.
The query selects id first in the tuple but it's not clear from the surrounding code why the ordering changed. The previous query returned (image_tag, markdown, shell, owner_id) but now returns (id, image_tag, markdown, shell, owner_id, embed_key). While this is functionally correct, inserting id at the beginning changes all the tuple indices which could be error-prone.
Consider adding the new id field at the end of the tuple or adding explicit field names in a comment to make the query more maintainable and less prone to ordering errors.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…security bypasses Co-authored-by: Yashb404 <139128977+Yashb404@users.noreply.github.com>
Co-authored-by: Yashb404 <139128977+Yashb404@users.noreply.github.com>
Co-authored-by: Yashb404 <139128977+Yashb404@users.noreply.github.com>
Co-authored-by: Yashb404 <139128977+Yashb404@users.noreply.github.com>
Add CSRF protection to whitelist management endpoints
Guard whitelist fetch with ownership check
…e-9d33-4b96-b406-7451d177ea95 Add VIP key authentication support to embed pages
…5-650a-40e1-90e7-81e27b77b3c7 Separate embed_key retrieval into dedicated authenticated endpoint
- dashboard.rs: Handle JSON parse failure explicitly to prevent stuck loading state - create.rs: Separate spawn error state from container_id to avoid invalid WebSocket connections Co-authored-by: Yashb404 <139128977+Yashb404@users.noreply.github.com>
Co-authored-by: Yashb404 <139128977+Yashb404@users.noreply.github.com>
Co-authored-by: Yashb404 <139128977+Yashb404@users.noreply.github.com>
…3-dbfa-4fbd-b82d-4f9b2eee664a Reorder query tuple fields to prevent index drift
…e-time Add rate limiting and storage limits to whitelist endpoints
…work Show loading state when VIP link is unavailable and fix error handling
|
@copilot Check again , merged all recommended changes |
Co-authored-by: Yashb404 <139128977+Yashb404@users.noreply.github.com>
…work Restore VIP link loading state lost during merge
Yashb404
requested review from
Rakshat28 and
karthikeyjoshi
and removed request for
Rakshat28
