-
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 2 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, locate_html_entrypoint}; | ||||||||||||||||||||||
| use crate::file_staging::FileStaging; | ||||||||||||||||||||||
| use anyhow::Result; | ||||||||||||||||||||||
| use serde::{Deserialize, Serialize}; | ||||||||||||||||||||||
|
|
@@ -124,7 +125,11 @@ async fn notify_upload_complete( | |||||||||||||||||||||
| Ok(result) | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| pub async fn handle_build_push(config_path: PathBuf, verbose: bool, message: Option<String>) -> Result<()> { | ||||||||||||||||||||||
| pub async fn handle_build_push( | ||||||||||||||||||||||
| config_path: PathBuf, | ||||||||||||||||||||||
| verbose: bool, | ||||||||||||||||||||||
| message: Option<String>, | ||||||||||||||||||||||
| ) -> Result<()> { | ||||||||||||||||||||||
| // Load wavedash.toml config | ||||||||||||||||||||||
| let wavedash_config = WavedashConfig::load(&config_path)?; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
|
|
@@ -157,14 +162,39 @@ 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 engine_version = wavedash_config.engine_version(); | ||||||||||||||||||||||
| let entrypoint_params = match engine_kind { | ||||||||||||||||||||||
| Some(EngineKind::Defold) => { | ||||||||||||||||||||||
| let html_entrypoint = locate_html_entrypoint(&upload_dir); | ||||||||||||||||||||||
| let html_path = html_entrypoint.as_deref().ok_or_else(|| { | ||||||||||||||||||||||
| anyhow::anyhow!("No HTML file found in upload_dir; required for DEFOLD builds") | ||||||||||||||||||||||
| })?; | ||||||||||||||||||||||
| let ver = engine_version | ||||||||||||||||||||||
| .ok_or_else(|| anyhow::anyhow!("DEFOLD engine requires a version"))?; | ||||||||||||||||||||||
| let html_relative_path = html_path | ||||||||||||||||||||||
| .strip_prefix(&upload_dir) | ||||||||||||||||||||||
| .unwrap_or(html_path) | ||||||||||||||||||||||
| .to_string_lossy() | ||||||||||||||||||||||
| .replace('\\', "/"); | ||||||||||||||||||||||
|
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.
Easiest fix: canonicalize 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.
Either canonicalize let upload_dir = upload_dir.canonicalize().with_context(|| {
format!("Failed to canonicalize upload_dir: {}", upload_dir.display())
})?;
// ...
let html_relative_path = html_path
.strip_prefix(&upload_dir)
.with_context(|| format!("HTML entrypoint {} is not under upload_dir {}", html_path.display(), upload_dir.display()))?
.to_string_lossy()
.replace('\\', "/");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.
Mirror the let upload_dir = upload_dir.canonicalize().with_context(|| {
format!("Failed to canonicalize upload_dir: {}", upload_dir.display())
})?;
// ...
let html_relative_path = html_path
.strip_prefix(&upload_dir)
.with_context(|| format!("HTML entrypoint {} is not under upload_dir {}", html_path.display(), upload_dir.display()))?
.to_string_lossy()
.replace('\\', "/");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. Regression in Commit What goes wrong now:
Two fixes; either is fine: Option A — restore the canonicalize block above let upload_dir = upload_dir
.canonicalize()
.with_context(|| format!("Failed to canonicalize upload_dir: {}", upload_dir.display()))?;(also restore Option B — replace the
Suggested change
|
||||||||||||||||||||||
| Some( | ||||||||||||||||||||||
| fetch_entrypoint_params("DEFOLD", ver, html_path, Some(&html_relative_path)) | ||||||||||||||||||||||
|
cloud9c marked this conversation as resolved.
Outdated
cloud9c marked this conversation as resolved.
Outdated
|
||||||||||||||||||||||
| .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() | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| _ => None, | ||||||||||||||||||||||
|
cloud9c marked this conversation as resolved.
Outdated
cloud9c marked this conversation as resolved.
Outdated
cloud9c marked this conversation as resolved.
Outdated
|
||||||||||||||||||||||
| }; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| // 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(), | ||||||||||||||||||||||
| engine_version, | ||||||||||||||||||||||
| wavedash_config.entrypoint(), | ||||||||||||||||||||||
| wavedash_config.executable_entrypoint_params(), | ||||||||||||||||||||||
| entrypoint_params, | ||||||||||||||||||||||
| message.as_deref(), | ||||||||||||||||||||||
| total_bytes, | ||||||||||||||||||||||
| &api_key, | ||||||||||||||||||||||
|
|
@@ -191,10 +221,7 @@ pub async fn handle_build_push(config_path: PathBuf, verbose: bool, message: Opt | |||||||||||||||||||||
|
|
||||||||||||||||||||||
| // Print the play URL | ||||||||||||||||||||||
| let site_host = config::get("open_browser_website_host")?; | ||||||||||||||||||||||
| let play_url = format!( | ||||||||||||||||||||||
| "{}/playtest/{}/{}", | ||||||||||||||||||||||
| site_host, result.game_slug, creds.uuid | ||||||||||||||||||||||
| ); | ||||||||||||||||||||||
| let play_url = format!("{}/playtest/{}/{}", site_host, result.game_slug, creds.uuid); | ||||||||||||||||||||||
| println!("\n▶ Play at: {}", play_url); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| Ok(()) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,12 +14,16 @@ struct EntrypointParamsResponse { | |
| entrypoint_params: Value, | ||
| } | ||
|
|
||
| // Shared by the Godot/Unity/Defold dev and build flows. A root-level `index.html` | ||
| // short-circuits, so single-HTML exports (Godot/Unity) keep their old behavior; the | ||
| // mtime/architecture ranking below only matters for nested multi-HTML dists (Defold). | ||
| pub fn locate_html_entrypoint(upload_dir: &Path) -> Option<PathBuf> { | ||
| let default_index = upload_dir.join("index.html"); | ||
| if default_index.is_file() { | ||
| return Some(default_index); | ||
| } | ||
|
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 new comment documents the early-return as intentional for single-HTML engines, but the short-circuit still applies to Defold and is a real footgun there. If a Defold project has any Given the comment now explicitly distinguishes "single-HTML exports (Godot/Unity)" from "nested multi-HTML dists (Defold)", consider gating the short-circuit on engine kind, e.g.: pub fn locate_html_entrypoint(upload_dir: &Path, engine: Option<EngineKind>) -> Option<PathBuf> {
if !matches!(engine, Some(EngineKind::Defold)) {
let default_index = upload_dir.join("index.html");
if default_index.is_file() {
return Some(default_index);
}
}
// ... walk + sort for Defold
}Or, weaker but still safer: prefer
Comment on lines
22
to
26
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. Root The comment at lines 17–18 documents the intent: this short-circuit is for "single-HTML" exports (Godot/Unity) and the ranking below is "for nested multi-HTML Defold dists." But the short-circuit doesn't check engine kind — it runs for everyone. So in a Defold project:
For Defold migrations specifically this is a real footgun — a pub fn locate_html_entrypoint(upload_dir: &Path, engine: Option<EngineKind>) -> Option<PathBuf> {
if !matches!(engine, Some(EngineKind::Defold)) {
let default_index = upload_dir.join("index.html");
if default_index.is_file() {
return Some(default_index);
}
}
// ... walk + sort for Defold (and as a fallback)
}Callers in |
||
|
|
||
| let mut html_files: Vec<PathBuf> = Vec::new(); | ||
|
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 unchanged early-return on lines 17–21 short-circuits the new architecture-aware selection. If For a Defold project migrating onto wavedash, this is a real footgun: any pre-existing root |
||
| for entry in WalkDir::new(upload_dir) | ||
| .min_depth(1) | ||
| .into_iter() | ||
|
|
@@ -28,23 +32,54 @@ pub fn locate_html_entrypoint(upload_dir: &Path) -> Option<PathBuf> { | |
| if entry.file_type().is_file() { | ||
| if let Some(ext) = entry.path().extension() { | ||
| if ext.eq_ignore_ascii_case("html") { | ||
| return Some(entry.into_path()); | ||
| html_files.push(entry.into_path()); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| None | ||
| html_files.sort_by(|a, b| { | ||
| let modified_a = a.metadata().and_then(|m| m.modified()).ok(); | ||
| let modified_b = b.metadata().and_then(|m| m.modified()).ok(); | ||
|
cloud9c marked this conversation as resolved.
Outdated
cloud9c marked this conversation as resolved.
Outdated
|
||
| let relative_a = a | ||
| .strip_prefix(upload_dir) | ||
| .unwrap_or(a) | ||
| .to_string_lossy() | ||
| .replace('\\', "/"); | ||
| let relative_b = b | ||
| .strip_prefix(upload_dir) | ||
| .unwrap_or(b) | ||
| .to_string_lossy() | ||
| .replace('\\', "/"); | ||
| let architecture_score = |relative: &str| { | ||
| if relative.starts_with("wasm-web/") { | ||
| 0 | ||
| } else if relative.starts_with("js-web/") { | ||
| 1 | ||
| } else { | ||
| 2 | ||
| } | ||
| }; | ||
|
cloud9c marked this conversation as resolved.
Outdated
cloud9c marked this conversation as resolved.
Outdated
cloud9c marked this conversation as resolved.
Outdated
|
||
|
|
||
| modified_b | ||
| .cmp(&modified_a) | ||
| .then_with(|| architecture_score(&relative_a).cmp(&architecture_score(&relative_b))) | ||
| .then_with(|| relative_a.cmp(&relative_b)) | ||
|
cloud9c marked this conversation as resolved.
Outdated
cloud9c marked this conversation as resolved.
Outdated
|
||
| }); | ||
|
cloud9c marked this conversation as resolved.
Outdated
|
||
|
|
||
| html_files.into_iter().next() | ||
| } | ||
|
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) -> Result<Value> { | ||
| 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 client = config::create_http_client()?; | ||
| let response = client | ||
|
|
@@ -53,6 +88,7 @@ pub async fn fetch_entrypoint_params(engine: &str, engine_version: &str, html_pa | |
| "engine": engine, | ||
| "engineVersion": engine_version, | ||
| "htmlContent": html_content, | ||
| "htmlPath": html_relative_path, | ||
|
cursor[bot] marked this conversation as resolved.
Outdated
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.
To make 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 response = client.post(&endpoint).json(&body).send().await
.with_context(|| "Failed to call CLI entrypoint params endpoint")?;Combined with the dev/mod.rs change to pass |
||
| })) | ||
| .send() | ||
| .await | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,7 +9,7 @@ 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}; | ||
|
|
@@ -99,7 +99,10 @@ pub async fn handle_dev(config_path: Option<PathBuf>, verbose: bool) -> Result<( | |
| // pinned to the dev-app source dir, so a relative `./dist` would resolve | ||
| // there (and serve the dev-app's own bundled `main.js`). | ||
| let upload_dir = upload_dir.canonicalize().with_context(|| { | ||
| format!("Failed to canonicalize upload_dir: {}", upload_dir.display()) | ||
| format!( | ||
| "Failed to canonicalize upload_dir: {}", | ||
| upload_dir.display() | ||
| ) | ||
| })?; | ||
|
|
||
| let engine_kind = wavedash_config.engine_type()?; | ||
|
|
@@ -111,7 +114,7 @@ pub async fn handle_dev(config_path: Option<PathBuf>, verbose: bool) -> Result<( | |
| 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) => { | ||
| Some(EngineKind::Godot | EngineKind::Unity | EngineKind::Defold) => { | ||
| let engine_label = engine_kind.unwrap().as_label(); | ||
| let html_path = html_entrypoint.as_deref().ok_or_else(|| { | ||
| anyhow::anyhow!( | ||
|
|
@@ -121,7 +124,15 @@ pub async fn handle_dev(config_path: Option<PathBuf>, verbose: bool) -> Result<( | |
| })?; | ||
| 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?) | ||
| let html_relative_path = html_path | ||
| .strip_prefix(&upload_dir) | ||
| .unwrap_or(html_path) | ||
| .to_string_lossy() | ||
| .replace('\\', "/"); | ||
| Some( | ||
| fetch_entrypoint_params(engine_label, ver, html_path, Some(&html_relative_path)) | ||
| .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::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;.)