-
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 7 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}; | ||||||||||||||||||||||
|
|
@@ -157,14 +158,47 @@ 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( | ||||||||||||||||||||||
| 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(), | ||||||||||||||||||||||
| engine_version, | ||||||||||||||||||||||
| wavedash_config.entrypoint(), | ||||||||||||||||||||||
| wavedash_config.executable_entrypoint_params(), | ||||||||||||||||||||||
| entrypoint_params, | ||||||||||||||||||||||
| message.as_deref(), | ||||||||||||||||||||||
| total_bytes, | ||||||||||||||||||||||
| &api_key, | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,12 +14,15 @@ struct EntrypointParamsResponse { | |
| entrypoint_params: Value, | ||
| } | ||
|
|
||
| // A root `index.html` short-circuits; the ranking below only matters for nested | ||
| // multi-HTML Defold dists. | ||
| 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); | ||
| } | ||
|
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,16 +31,60 @@ 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 | ||
| // Match the arch folder as any path segment (Defold nests it under a game dir). | ||
| let architecture_score = |relative: &str| { | ||
| let mut segments = relative.split('/'); | ||
| if segments.clone().any(|s| s == "wasm-web") { | ||
| 0 | ||
| } else if segments.any(|s| s == "js-web") { | ||
| 1 | ||
| } else { | ||
| 2 | ||
| } | ||
| }; | ||
|
|
||
| // Compute each candidate's sort key once (stat + strip_prefix), so sorting | ||
| // doesn't re-stat on every comparison. | ||
| let mut ranked: Vec<(std::time::SystemTime, i32, String, PathBuf)> = html_files | ||
| .into_iter() | ||
| .map(|path| { | ||
| let modified = path | ||
| .metadata() | ||
| .and_then(|m| m.modified()) | ||
| .unwrap_or(std::time::UNIX_EPOCH); | ||
| let relative = path | ||
| .strip_prefix(upload_dir) | ||
| .unwrap_or(&path) | ||
| .to_string_lossy() | ||
| .replace('\\', "/"); | ||
| let arch = architecture_score(&relative); | ||
| (modified, arch, relative, path) | ||
| }) | ||
| .collect(); | ||
|
|
||
| // Newest export wins; wasm-web beats js-web only on ties, then path. Unreadable | ||
| // mtime falls back to UNIX_EPOCH (oldest) so it can't sort as "newest". | ||
| ranked.sort_by(|a, b| { | ||
| b.0.cmp(&a.0) | ||
| .then_with(|| a.1.cmp(&b.1)) | ||
| .then_with(|| a.2.cmp(&b.2)) | ||
| }); | ||
|
cloud9c marked this conversation as resolved.
Outdated
|
||
|
|
||
| ranked.into_iter().next().map(|(_, _, _, path)| path) | ||
| } | ||
|
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")?; | ||
|
|
@@ -53,6 +100,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}; | ||
|
|
@@ -111,7 +111,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 +121,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() | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -39,6 +39,7 @@ struct GamesResponse { | |
| enum EngineType { | ||
| Godot, | ||
| Unity, | ||
| Defold, | ||
| Custom, | ||
| } | ||
|
|
||
|
|
@@ -47,6 +48,8 @@ impl EngineType { | |
| match self { | ||
| EngineType::Godot => "build", | ||
| EngineType::Unity => "build", | ||
| // Defold reserves top-level build/ for its build cache, so default elsewhere. | ||
| EngineType::Defold => "dist", | ||
|
cloud9c marked this conversation as resolved.
cloud9c marked this conversation as resolved.
|
||
| EngineType::Custom => "dist", | ||
|
cloud9c marked this conversation as resolved.
|
||
| } | ||
| } | ||
|
|
@@ -130,10 +133,25 @@ fn detect_unity(dir: &Path) -> Option<DetectedEngine> { | |
| None | ||
| } | ||
|
|
||
| /// Look for a Defold project marker. | ||
| fn detect_defold(dir: &Path) -> Option<DetectedEngine> { | ||
| if dir.join("game.project").is_file() { | ||
| return Some(DetectedEngine { | ||
| engine_type: EngineType::Defold, | ||
| version_hint: None, | ||
| }); | ||
| } | ||
|
|
||
| None | ||
| } | ||
|
cloud9c marked this conversation as resolved.
cloud9c marked this conversation as resolved.
cloud9c marked this conversation as resolved.
|
||
|
|
||
| fn detect_engine(dir: &Path) -> DetectedEngine { | ||
| if let Some(engine) = detect_godot(dir) { | ||
| return engine; | ||
| } | ||
| if let Some(engine) = detect_defold(dir) { | ||
| return engine; | ||
| } | ||
| if let Some(engine) = detect_unity(dir) { | ||
| return engine; | ||
| } | ||
|
|
@@ -233,6 +251,10 @@ fn generate_toml( | |
| let version = engine_version.unwrap_or("2022.3"); | ||
| toml.push_str(&format!("\n[unity]\nversion = \"{}\"\n", version)); | ||
| } | ||
| EngineType::Defold => { | ||
| let version = engine_version.unwrap_or("1.12.4"); | ||
| toml.push_str(&format!("\n[defold]\nversion = \"{}\"\n", version)); | ||
| } | ||
|
Comment on lines
+254
to
+263
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 error message is clear once you trigger it, but the UX cliff is:
Two cheaper alternatives, in order of effort:
Right now 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 comment at lines 256–258 documents the design ("the dev fills in the export they want to ship;
This is the only engine where the next step the outro promises doesn't work without manual file editing. Two cheap mitigations, in order of effort:
Either is fine — the goal is just to make the cliff visible before the user hits it. Acknowledged in the previous review pass; flagging once more since the latest commit ("edge cases") had room to address it. |
||
| EngineType::Custom => { | ||
| toml.push_str("\nentrypoint = \"index.html\"\n"); | ||
| } | ||
|
|
@@ -279,7 +301,7 @@ pub async fn handle_init() -> Result<()> { | |
| let current_dir = std::env::current_dir()?; | ||
| let detected = detect_engine(¤t_dir); | ||
|
|
||
| // Only prompt for version when we detect Godot/Unity. | ||
| // Only prompt for version when we detect a first-class engine. | ||
| // For web builds (threejs, phaser, custom, etc.) no engine config is needed. | ||
| let engine_version: Option<String> = match &detected.engine_type { | ||
| EngineType::Godot => { | ||
|
|
@@ -302,6 +324,13 @@ pub async fn handle_init() -> Result<()> { | |
| Some(version) | ||
| } | ||
| } | ||
| EngineType::Defold => { | ||
| let version: String = cliclack::input("Defold version") | ||
| .placeholder("1.12.4") | ||
| .default_input("1.12.4") | ||
| .interact()?; | ||
| Some(version) | ||
| } | ||
|
cloud9c marked this conversation as resolved.
|
||
| EngineType::Custom => None, | ||
| }; | ||
|
|
||
|
|
||
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;.)