diff --git a/scripts/game_state.gd b/scripts/game_state.gd index ab892c0..da2fc6f 100644 --- a/scripts/game_state.gd +++ b/scripts/game_state.gd @@ -42,15 +42,23 @@ func _ready() -> void: # ── Shared persistence plumbing ─────────────────────────────────────────────── # The web build stores a JSON string inside localStorage (hence the double # stringify/parse dance); the desktop build writes plain JSON files. These -# four helpers are the only place either quirk lives. +# helpers are the only place either quirk lives. + +# Completion value appended to the write eval statement: a fixed string +# literal evaluated after setItem. Needed because setItem returns undefined +# on success and JavaScriptBridge.eval converts both that value and a thrown +# quota/security error to null, so the bare call cannot signal success. +const WRITE_EVAL_SUCCESS_SENTINEL := "windowstead-write-ok" # Builds the JavaScript statement used to write `payload` to `key` in # localStorage. The key and payload are JSON-encoded so any quotes or # backslashes they contain cannot break out of the resulting JavaScript -# string literal. Exposed as a static helper so tests can assert on the -# exact eval'd string without invoking JavaScriptBridge. +# string literal. The trailing sentinel literal gives the statement a +# deterministic completion value (see WRITE_EVAL_SUCCESS_SENTINEL). +# Exposed as a static helper so tests can assert on the exact eval'd string +# without invoking JavaScriptBridge. static func build_local_storage_write_eval(key: String, payload: String) -> String: - return "localStorage.setItem(%s, %s)" % [JSON.stringify(key), JSON.stringify(payload)] + return "localStorage.setItem(%s, %s); %s" % [JSON.stringify(key), JSON.stringify(payload), JSON.stringify(WRITE_EVAL_SUCCESS_SENTINEL)] # Builds the JavaScript statement used to read `key` from localStorage. # See `build_local_storage_write_eval` for why the key is JSON-encoded. @@ -64,15 +72,14 @@ static func build_local_storage_remove_eval(key: String) -> String: func _local_storage_write(key: String, payload: String) -> bool: var result = JavaScriptBridge.eval(build_local_storage_write_eval(key, payload), true) - # localStorage.setItem returns undefined on success; on quota / security - # errors it throws and eval yields null. Anything other than a non-null, - # non-empty result counts as a failed write. - if result == null: - return false - var as_string := String(result) - if as_string.is_empty() or as_string == "null": - return false - return true + return local_storage_write_succeeded(result) + +# Decodes the completion value of the statement produced by +# build_local_storage_write_eval: only the exact sentinel string means the +# write succeeded. Throws (quota / security errors) and any other value are +# failures; `undefined` also arrives as null from JavaScriptBridge.eval. +static func local_storage_write_succeeded(result: Variant) -> bool: + return typeof(result) == TYPE_STRING and result == WRITE_EVAL_SUCCESS_SENTINEL func _local_storage_read(key: String) -> Dictionary: var raw = JavaScriptBridge.eval(build_local_storage_read_eval(key), true) diff --git a/tests/test_local_storage_xss.gd b/tests/test_local_storage_xss.gd index b7c008f..34ac2e5 100644 --- a/tests/test_local_storage_xss.gd +++ b/tests/test_local_storage_xss.gd @@ -12,13 +12,20 @@ extends "res://tests/test_case.gd" # 2. The exact JavaScript statement produced by the `build_local_storage_*` # helpers in game_state.gd is well-formed: each one is parseable as a # single function call whose JSON args round-trip back to the original -# key/payload values. This catches regressions like #291 where the key -# was concatenated directly into the eval string instead of being -# JSON-encoded. +# key/payload values. The write statement additionally ends with a +# fixed success-sentinel string literal (WRITE_STATEMENT_SUFFIX below) +# so the eval'd statement has a deterministic completion value. This +# catches regressions like #291 where the key was concatenated directly +# into the eval string instead of being JSON-encoded. # ============================================================================= const GameState := preload("res://scripts/game_state.gd") +# The fixed string-literal suffix appended to the write eval statement by +# build_local_storage_write_eval. A fixed literal, not attacker-controllable; +# tests strip it before round-tripping the setItem call. +const WRITE_STATEMENT_SUFFIX := "; \"windowstead-write-ok\"" + func run_tests() -> void: flow_json_stringify_escapes_single_quote() flow_json_stringify_escapes_backtick() @@ -31,6 +38,7 @@ func run_tests() -> void: flow_read_eval_statement_round_trips_evil_key() flow_remove_eval_statement_round_trips_evil_key() flow_eval_statement_has_single_call_shape() + flow_write_success_detection_matches_sentinel() # Verify that a key with a single quote is properly escaped by JSON.stringify. # The output should use double quotes around the string, so single quotes pass through safely. @@ -79,7 +87,9 @@ func flow_json_stringify_escapes_combined_special_chars() -> void: func flow_write_eval_statement_round_trips_evil_key() -> void: var evil_key = "key';alert(1);//with\"backticks`and\\slashes" var statement = GameState.build_local_storage_write_eval(evil_key, "{}") - var key_json = _extract_first_json_arg(statement, "localStorage.setItem(") + assert_true(statement.ends_with(WRITE_STATEMENT_SUFFIX), "write statement ends with the fixed success sentinel suffix") + var call = statement.substr(0, statement.length() - WRITE_STATEMENT_SUFFIX.length()) + var key_json = _extract_first_json_arg(call, "localStorage.setItem(") assert_ne(key_json, "", "write statement has a key argument") var parsed = JSON.parse_string(key_json) assert_eq(parsed, evil_key, "write eval statement round-trips an evil key") @@ -89,7 +99,9 @@ func flow_write_eval_statement_round_trips_evil_key() -> void: func flow_write_eval_statement_round_trips_evil_payload() -> void: var evil_payload = "{\"inject\":\"`); evil(); //\"}" var statement = GameState.build_local_storage_write_eval("SAVE_KEY", evil_payload) - var value_json = _extract_second_json_arg(statement, "localStorage.setItem(") + assert_true(statement.ends_with(WRITE_STATEMENT_SUFFIX), "write statement ends with the fixed success sentinel suffix") + var call = statement.substr(0, statement.length() - WRITE_STATEMENT_SUFFIX.length()) + var value_json = _extract_second_json_arg(call, "localStorage.setItem(") assert_ne(value_json, "", "write statement has a value argument") var parsed = JSON.parse_string(value_json) assert_eq(parsed, evil_payload, "write eval statement round-trips an evil payload") @@ -114,24 +126,41 @@ func flow_remove_eval_statement_round_trips_evil_key() -> void: var parsed = JSON.parse_string(key_json) assert_eq(parsed, evil_key, "remove eval statement round-trips an evil key") -# All three eval statements should be a single call with no trailing junk -# after the closing paren — i.e. the attacker can't append another statement -# to the eval'd string. +# The read/remove eval statements should be a single call with no trailing +# junk after the closing paren, and the write statement should be a single +# call plus the fixed success-sentinel suffix — i.e. the attacker can't +# append another statement to the eval'd string. func flow_eval_statement_has_single_call_shape() -> void: - # The first "(" must be the call-opening paren and the last char must be the - # call-closing ")". Any attacker-supplied parens are confined to the JSON - # string args and cannot terminate or extend the eval'd call. + # The first "(" must be the call-opening paren and the last char of the + # call part must be the call-closing ")". Any attacker-supplied parens are + # confined to the JSON string args and cannot terminate or extend the + # eval'd call. The write statement's trailing suffix is a fixed literal, + # not attacker-controllable. var evil_key = "key'); alert(1); (" var write = GameState.build_local_storage_write_eval(evil_key, "{}") var read = GameState.build_local_storage_read_eval(evil_key) var remove = GameState.build_local_storage_remove_eval(evil_key) assert_true(write.begins_with("localStorage.setItem("), "write call opens with localStorage.setItem(") - assert_true(write.ends_with(")"), "write call closes with a single trailing paren") + assert_true(write.ends_with(WRITE_STATEMENT_SUFFIX), "write statement ends with the fixed success sentinel suffix") + var write_call = write.substr(0, write.length() - WRITE_STATEMENT_SUFFIX.length()) + assert_true(write_call.ends_with(")"), "write call (suffix removed) closes with a single trailing paren") assert_true(read.begins_with("localStorage.getItem("), "read call opens with localStorage.getItem(") assert_true(read.ends_with(")"), "read call closes with a single trailing paren") assert_true(remove.begins_with("localStorage.removeItem("), "remove call opens with localStorage.removeItem(") assert_true(remove.ends_with(")"), "remove call closes with a single trailing paren") +# The write-success detector accepts only the exact sentinel completion +# value; throws (null), empty strings, the literal "null" string, and any +# other value are failures. +func flow_write_success_detection_matches_sentinel() -> void: + assert_true(GameState.local_storage_write_succeeded(GameState.WRITE_EVAL_SUCCESS_SENTINEL), "success sentinel is accepted") + assert_false(GameState.local_storage_write_succeeded(null), "null (throw / undefined) is a failure") + assert_false(GameState.local_storage_write_succeeded(""), "empty string is a failure") + assert_false(GameState.local_storage_write_succeeded("null"), "literal null string is a failure") + assert_false(GameState.local_storage_write_succeeded("ok"), "arbitrary string is a failure") + assert_false(GameState.local_storage_write_succeeded(false), "boolean false is a failure") + assert_false(GameState.local_storage_write_succeeded(0), "zero is a failure") + # Helper: extract the first JSON-string argument from a `prefix(...)` call. # The statement always ends with `)`, so we strip the prefix and the trailing # paren and pull out everything before the first top-level comma.