-
Notifications
You must be signed in to change notification settings - Fork 1
defold entrypoint #34
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 9 commits
611f94a
10f7bb0
b6db7b9
6cf8c12
a291f10
99f8b5d
1aa904f
c4ec363
64048d1
d852111
fb68137
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,6 @@ | ||
| use crate::auth::AuthManager; | ||
| use crate::config::{self, WavedashConfig}; | ||
| use crate::config::{self, EngineKind, WavedashConfig}; | ||
| use crate::dev::entrypoint::{fetch_entrypoint_params, resolve_defold_entrypoint}; | ||
| use crate::file_staging::FileStaging; | ||
| use anyhow::Result; | ||
| use serde::{Deserialize, Serialize}; | ||
|
|
@@ -157,14 +158,44 @@ pub async fn handle_build_push(config_path: PathBuf, verbose: bool, message: Opt | |
| anyhow::bail!("No files found in {}", upload_dir.display()); | ||
| } | ||
|
|
||
| // Get temporary R2 credentials (includes build size) | ||
| let engine_kind = wavedash_config.engine_type()?; | ||
| let entrypoint = match engine_kind { | ||
| Some(EngineKind::Defold) => wavedash_config.entrypoint.as_deref(), | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Slash-normalization drift between the
But at line 163 (and the parallel entrypoint = "wasm-web\\game\\index.html"the server receives It's a narrow case (most users will type forward slashes), but the back-slash handling in Cheapest fix: reuse the normalized |
||
| _ => wavedash_config.entrypoint(), | ||
| }; | ||
|
cursor[bot] marked this conversation as resolved.
|
||
| let engine_version = wavedash_config.engine_version(); | ||
| let entrypoint_params = match engine_kind { | ||
| Some(EngineKind::Defold) => { | ||
| let (html_path, html_relative_path) = | ||
| resolve_defold_entrypoint(&upload_dir, wavedash_config.entrypoint.as_deref())?; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For Defold, Trace for a user who ran
For dev/mod.rs ( Moving |
||
| let ver = engine_version | ||
| .ok_or_else(|| anyhow::anyhow!("DEFOLD engine requires a version"))?; | ||
| Some( | ||
| fetch_entrypoint_params( | ||
| EngineKind::Defold.as_label(), | ||
| ver, | ||
| &html_path, | ||
| Some(&html_relative_path), | ||
| ) | ||
| .await?, | ||
| ) | ||
|
cloud9c marked this conversation as resolved.
|
||
| } | ||
|
Comment on lines
+178
to
+193
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Result: a user who sets Two ways to close this:
Either way, the gap is in |
||
| Some(EngineKind::JsDos | EngineKind::Ruffle | EngineKind::RenPy) => { | ||
| wavedash_config.executable_entrypoint_params() | ||
| } | ||
| // Explicit (not `_`) so a new EngineKind forces a decision. Godot/Unity | ||
| // params are computed server-side. | ||
| Some(EngineKind::Godot | EngineKind::Unity) => None, | ||
| None => None, | ||
| }; | ||
|
|
||
| // Get temporary R2 credentials (includes build size) | ||
| let creds = get_temp_credentials( | ||
| &wavedash_config.game_id, | ||
| engine_kind.map(|e| e.as_label()), | ||
| wavedash_config.engine_version(), | ||
| wavedash_config.entrypoint(), | ||
| wavedash_config.executable_entrypoint_params(), | ||
| engine_version, | ||
| entrypoint, | ||
| entrypoint_params, | ||
| message.as_deref(), | ||
| total_bytes, | ||
| &api_key, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| use std::fs; | ||
| use std::path::{Path, PathBuf}; | ||
| use std::path::{Component, Path, PathBuf}; | ||
|
|
||
| use anyhow::{Context, Result}; | ||
| use serde::Deserialize; | ||
|
|
@@ -14,6 +14,11 @@ struct EntrypointParamsResponse { | |
| entrypoint_params: Value, | ||
| } | ||
|
|
||
| /// Locate the HTML entrypoint for engines that ship a single root export | ||
| /// (Godot, Unity): a root `index.html`, falling back to the first `.html` found. | ||
| /// Defold doesn't use this — it parses the explicit `entrypoint` from | ||
| /// wavedash.toml, since its bundles can nest the HTML under `wasm-web`/`js-web` | ||
| /// and ship more than one. No "which export is newest" guessing lives here. | ||
| pub fn locate_html_entrypoint(upload_dir: &Path) -> Option<PathBuf> { | ||
| let default_index = upload_dir.join("index.html"); | ||
| if default_index.is_file() { | ||
|
|
@@ -37,23 +42,94 @@ pub fn locate_html_entrypoint(upload_dir: &Path) -> Option<PathBuf> { | |
| None | ||
| } | ||
|
|
||
| pub async fn fetch_entrypoint_params(engine: &str, engine_version: &str, html_path: &Path) -> Result<Value> { | ||
| /// Resolve the developer-named Defold entrypoint HTML to its absolute path plus | ||
| /// normalized build-relative path. Defold names its entrypoint explicitly (a | ||
| /// bundle can ship both `wasm-web/` and `js-web/`), so there's no inference here | ||
| /// — a missing or wrong path is a clear error, not a guess. | ||
| pub fn resolve_defold_entrypoint( | ||
| upload_dir: &Path, | ||
| entrypoint: Option<&str>, | ||
| ) -> Result<(PathBuf, String)> { | ||
| let entrypoint = entrypoint.ok_or_else(|| { | ||
| anyhow::anyhow!( | ||
| "Defold builds need an `entrypoint` in wavedash.toml pointing to your HTML5 export, e.g.\n entrypoint = \"wasm-web/<game>/index.html\"\n(a Defold bundle can contain both wasm-web/ and js-web/ — pick one)" | ||
| ) | ||
| })?; | ||
| let relative_path = entrypoint.replace('\\', "/"); | ||
| let entrypoint_path = Path::new(&relative_path); | ||
| if entrypoint_path.is_absolute() | ||
| || entrypoint_path.components().any(|component| { | ||
| matches!( | ||
| component, | ||
| Component::ParentDir | Component::RootDir | Component::Prefix(_) | ||
| ) | ||
| }) | ||
| { | ||
| anyhow::bail!( | ||
| "Defold entrypoint `{}` must be a relative path inside upload_dir ({}).", | ||
| entrypoint, | ||
| upload_dir.display() | ||
| ); | ||
| } | ||
|
|
||
| let lower = relative_path.to_ascii_lowercase(); | ||
| if !lower.ends_with(".html") && !lower.ends_with(".htm") { | ||
| anyhow::bail!( | ||
| "Defold entrypoint `{}` must be an HTML file (.html/.htm).", | ||
| entrypoint | ||
| ); | ||
| } | ||
|
|
||
| let html_path = upload_dir.join(&relative_path); | ||
| if !html_path.is_file() { | ||
| anyhow::bail!( | ||
| "Defold entrypoint `{}` not found under {}. Point `entrypoint` in wavedash.toml at your export's index.html.", | ||
| entrypoint, | ||
| upload_dir.display() | ||
| ); | ||
| } | ||
|
|
||
| let canonical_upload_dir = upload_dir | ||
| .canonicalize() | ||
| .with_context(|| format!("Failed to canonicalize {}", upload_dir.display()))?; | ||
| let canonical_html_path = html_path | ||
| .canonicalize() | ||
| .with_context(|| format!("Failed to canonicalize {}", html_path.display()))?; | ||
| if !canonical_html_path.starts_with(&canonical_upload_dir) { | ||
| anyhow::bail!( | ||
| "Defold entrypoint `{}` resolves outside upload_dir ({}).", | ||
| entrypoint, | ||
| upload_dir.display() | ||
| ); | ||
| } | ||
|
|
||
| Ok((html_path, relative_path)) | ||
|
cursor[bot] marked this conversation as resolved.
|
||
| } | ||
|
Comment on lines
+49
to
+116
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Two concrete misconfigurations:
The user "owns their own machine" so this isn't a security exploit, but it surfaces typos and copy-paste mistakes as silent wrong-file uploads instead of clear errors. A cheap guard: canonicalize both and check containment: let html_path = upload_dir.join(&relative_path);
if !html_path.is_file() {
anyhow::bail!(/* same as today */);
}
// Reject anything that escapes upload_dir.
let canon_html = html_path
.canonicalize()
.with_context(|| format!("Failed to canonicalize {}", html_path.display()))?;
let canon_upload = upload_dir
.canonicalize()
.with_context(|| format!("Failed to canonicalize {}", upload_dir.display()))?;
if !canon_html.starts_with(&canon_upload) {
anyhow::bail!(
"Defold entrypoint `{}` resolves outside upload_dir ({}). The path must be relative to upload_dir and stay inside it.",
entrypoint,
upload_dir.display()
);
}Bonus: also reject paths whose lowercase extension isn't |
||
|
|
||
| pub async fn fetch_entrypoint_params( | ||
| engine: &str, | ||
| engine_version: &str, | ||
| html_path: &Path, | ||
| html_relative_path: Option<&str>, | ||
| ) -> Result<Value> { | ||
| let html_content = fs::read_to_string(html_path) | ||
| .with_context(|| format!("Failed to read {}", html_path.display()))?; | ||
| let api_host = config::get("api_host")?; | ||
| let endpoint = format!( | ||
| "{}/cli/entrypoint-params", | ||
| api_host.trim_end_matches('/') | ||
| ); | ||
| let endpoint = format!("{}/cli/entrypoint-params", api_host.trim_end_matches('/')); | ||
|
|
||
| let mut body = serde_json::json!({ | ||
| "engine": engine, | ||
| "engineVersion": engine_version, | ||
| "htmlContent": html_content, | ||
| }); | ||
| if let Some(html_path) = html_relative_path { | ||
| body["htmlPath"] = serde_json::json!(html_path); | ||
| } | ||
|
|
||
| let client = config::create_http_client()?; | ||
| let response = client | ||
| .post(&endpoint) | ||
| .json(&serde_json::json!({ | ||
| "engine": engine, | ||
| "engineVersion": engine_version, | ||
| "htmlContent": html_content, | ||
| })) | ||
| .json(&body) | ||
| .send() | ||
| .await | ||
| .with_context(|| "Failed to call CLI entrypoint params endpoint")?; | ||
|
|
@@ -70,3 +146,65 @@ pub async fn fetch_entrypoint_params(engine: &str, engine_version: &str, html_pa | |
|
|
||
| Ok(parsed.entrypoint_params) | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
| use std::time::{SystemTime, UNIX_EPOCH}; | ||
|
|
||
| fn temp_upload_dir(name: &str) -> PathBuf { | ||
| let unique = SystemTime::now() | ||
| .duration_since(UNIX_EPOCH) | ||
| .expect("system clock before unix epoch") | ||
| .as_nanos(); | ||
| std::env::temp_dir().join(format!( | ||
| "wavedash-cli-entrypoint-{name}-{}-{unique}", | ||
| std::process::id() | ||
| )) | ||
| } | ||
|
|
||
| #[test] | ||
| fn resolves_defold_entrypoint_inside_upload_dir() { | ||
| let upload_dir = temp_upload_dir("inside"); | ||
| let html_path = upload_dir.join("wasm-web/example/index.html"); | ||
| fs::create_dir_all(html_path.parent().expect("html parent")).expect("create dirs"); | ||
| fs::write(&html_path, "<html></html>").expect("write html"); | ||
|
|
||
| let (resolved, relative) = | ||
| resolve_defold_entrypoint(&upload_dir, Some("wasm-web/example/index.html")) | ||
| .expect("resolve entrypoint"); | ||
|
|
||
| assert_eq!(resolved, html_path); | ||
| assert_eq!(relative, "wasm-web/example/index.html"); | ||
|
|
||
| fs::remove_dir_all(upload_dir).expect("cleanup"); | ||
| } | ||
|
|
||
| #[test] | ||
| fn rejects_defold_entrypoint_escape() { | ||
| let upload_dir = temp_upload_dir("escape"); | ||
| fs::create_dir_all(&upload_dir).expect("create upload dir"); | ||
|
|
||
| let err = resolve_defold_entrypoint(&upload_dir, Some("../index.html")) | ||
| .expect_err("escape should fail"); | ||
|
|
||
| assert!(err.to_string().contains("relative path inside upload_dir")); | ||
|
|
||
| fs::remove_dir_all(upload_dir).expect("cleanup"); | ||
| } | ||
|
|
||
| #[test] | ||
| fn rejects_defold_entrypoint_non_html() { | ||
| let upload_dir = temp_upload_dir("non-html"); | ||
| let asset_path = upload_dir.join("wasm-web/example/something.png"); | ||
| fs::create_dir_all(asset_path.parent().expect("asset parent")).expect("create dirs"); | ||
| fs::write(&asset_path, "not html").expect("write asset"); | ||
|
|
||
| let err = resolve_defold_entrypoint(&upload_dir, Some("wasm-web/example/something.png")) | ||
| .expect_err("non-html entrypoint should fail"); | ||
|
|
||
| assert!(err.to_string().contains("HTML file")); | ||
|
|
||
| fs::remove_dir_all(upload_dir).expect("cleanup"); | ||
| } | ||
|
Comment on lines
+175
to
+252
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Test cleanup leaks temp directories if any assertion before Each test ends with This won't bite production users — only test infrastructure — but it makes it harder to diagnose flaky-test situations on a long-lived runner.
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,11 +9,11 @@ use crate::config::{self, EngineKind, WavedashConfig}; | |
| use crate::file_staging::FileStaging; | ||
|
|
||
| mod dev_app; | ||
| mod entrypoint; | ||
| pub(crate) mod entrypoint; | ||
| mod launcher; | ||
|
|
||
| use dev_app::{ensure_dev_app, user_data_dir}; | ||
| use entrypoint::{fetch_entrypoint_params, locate_html_entrypoint}; | ||
| use entrypoint::{fetch_entrypoint_params, locate_html_entrypoint, resolve_defold_entrypoint}; | ||
| use launcher::{run_dev_app, DevAppConfig}; | ||
|
|
||
| #[derive(Debug, Deserialize)] | ||
|
|
@@ -104,24 +104,41 @@ pub async fn handle_dev(config_path: Option<PathBuf>, verbose: bool) -> Result<( | |
|
|
||
| let engine_kind = wavedash_config.engine_type()?; | ||
|
|
||
| let entrypoint = wavedash_config.entrypoint().map(|s| s.to_string()); | ||
| let entrypoint = match engine_kind { | ||
| Some(EngineKind::Defold) => wavedash_config.entrypoint.as_deref().map(str::to_string), | ||
| _ => wavedash_config.entrypoint().map(str::to_string), | ||
| }; | ||
|
|
||
| FileStaging::prepare(&upload_dir, &wavedash_config)?; | ||
|
|
||
| let html_entrypoint = locate_html_entrypoint(&upload_dir); | ||
| let engine_version = wavedash_config.engine_version(); | ||
| let entrypoint_params = match engine_kind { | ||
| Some(EngineKind::Godot | EngineKind::Unity) => { | ||
| let engine_label = engine_kind.unwrap().as_label(); | ||
| let html_path = html_entrypoint.as_deref().ok_or_else(|| { | ||
| Some(kind @ (EngineKind::Godot | EngineKind::Unity)) => { | ||
| let engine_label = kind.as_label(); | ||
| let html_path = locate_html_entrypoint(&upload_dir).ok_or_else(|| { | ||
| anyhow::anyhow!( | ||
| "No HTML file found in upload_dir; required for {} builds", | ||
| engine_label | ||
| ) | ||
| })?; | ||
| let ver = engine_version | ||
| .ok_or_else(|| anyhow::anyhow!("{} engine requires a version", engine_label))?; | ||
| Some(fetch_entrypoint_params(engine_label, ver, html_path).await?) | ||
| Some(fetch_entrypoint_params(engine_label, ver, &html_path, None).await?) | ||
| } | ||
|
Comment on lines
+125
to
+136
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The Godot/Unity branch (lines 113–132) computes Suggested isolation: keep Some(kind @ (EngineKind::Godot | EngineKind::Unity)) => {
let engine_label = kind.as_label();
let html_path = locate_html_entrypoint(&upload_dir).ok_or_else(|| {
anyhow::anyhow!(
"No HTML file found in upload_dir; required for {} builds",
engine_label
)
})?;
let ver = engine_version
.ok_or_else(|| anyhow::anyhow!("{} engine requires a version", engine_label))?;
Some(fetch_entrypoint_params(engine_label, ver, &html_path, None).await?)
}This also makes the |
||
| Some(EngineKind::Defold) => { | ||
| let (html_path, html_relative_path) = | ||
| resolve_defold_entrypoint(&upload_dir, wavedash_config.entrypoint.as_deref())?; | ||
| let ver = engine_version | ||
| .ok_or_else(|| anyhow::anyhow!("DEFOLD engine requires a version"))?; | ||
| Some( | ||
| fetch_entrypoint_params( | ||
| EngineKind::Defold.as_label(), | ||
| ver, | ||
| &html_path, | ||
| Some(&html_relative_path), | ||
| ) | ||
| .await?, | ||
| ) | ||
| } | ||
| Some(EngineKind::JsDos | EngineKind::Ruffle | EngineKind::RenPy) => { | ||
| wavedash_config.executable_entrypoint_params() | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
upload_diris still not canonicalized inhandle_build_push— onlyresolve_defold_entrypoint's internal canonicalize-containment check runs, whiledev/mod.rs:101canonicalizes once and the rest of the flow operates on a clean absolute path.This was raised in a prior review and is moot for path-escape safety —
resolve_defold_entrypointre-canonicalizes both sides before itsstarts_withcheck (entrypoint.rs:92-104), so escapes are rejected. But the broader symmetry concern still applies:html_pathat entrypoint.rs:106 is the un-canonicalizedupload_dir.join(relative_path). It's read viafs::read_to_string(fine), but it flows intofetch_entrypoint_paramsand any error messages downstream — the two flows disagree on the textual path for the same project.wavedash devandwavedash build pushfor the same misconfiguration —devreports a/private/var/...absolute path;build pushreports./build/default/.... That asymmetry makes user-reported failures harder to triage.R2Uploaderat line 215 walks the non-canonical path; pre-PR behavior, but worth confirming the new Defold flow doesn't introduce sensitivity to symlinks underupload_dir.Restoring the canonicalize block (the same one added in
a291f10and removed in1aa904f) is the simplest fix and keeps the two flows symmetric:(requires restoring
use anyhow::Context;.)