Conversation
This commit adds a security check to the `loadPersistedState` function to ensure that the data retrieved from `localStorage` and parsed with `JSON.parse` is a non-null, non-array object. This prevents potential runtime errors and logic bypasses if `localStorage` is tampered with or contains unexpected JSON values (like `null`, `number`, or `array`). Additionally, this commit implements type validation for the following boolean fields in `PersistedState`: - `showBuildings` - `showClouds` - `syncToRealTime` These fields are now checked to ensure they are actual booleans before being returned. Verification: - Created a standalone test script `scripts/test-min.ts` that mocks `window` and `localStorage` to test various invalid and valid JSON inputs. - Confirmed that `loadPersistedState` correctly handles invalid inputs by returning an empty object and handles valid inputs as expected. - Verified that boolean fields are correctly validated. Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
There was a problem hiding this comment.
Pull request overview
Adds defensive validation around persisted UI state loaded from localStorage so corrupted/tampered values don’t propagate into runtime state.
Changes:
- Reject non-object JSON parse results (null/arrays/primitives) when loading persisted state.
- Validate boolean flags (
showBuildings,showClouds,syncToRealTime) before returning persisted state. - Add a minimal script intended to manually exercise
loadPersistedState().
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/lib/core/persistence.ts | Adds runtime shape/type validation for parsed persisted state (object + boolean flags). |
| scripts/test-min.ts | Adds a minimal Node script to try loading persisted state with mocked storage. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (parsed.location && !LOCATION_IDS.has(parsed.location)) { | ||
| delete parsed.location; | ||
| } | ||
| const VALID_WEATHER: WeatherType[] = ['clear', 'cloudy', 'rain', 'overcast', 'storm']; | ||
| if (parsed.weather && !VALID_WEATHER.includes(parsed.weather)) { | ||
| delete parsed.weather; | ||
| } |
There was a problem hiding this comment.
location/weather sanitization currently uses truthiness checks (if (parsed.location && …), if (parsed.weather && …)), which allows invalid but falsy values (e.g., '', 0, false) to bypass validation and be returned in the persisted state object. Consider switching to explicit !== undefined checks and validating the type is string before applying LOCATION_IDS.has(...) / VALID_WEATHER.includes(...), deleting the key otherwise.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
There was a problem hiding this comment.
I have updated the boolean validation logic to avoid using a loop with keyof PersistedState, which can sometimes cause type-level issues or be less clear. I am now validating each boolean flag explicitly.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
- Implement explicit type checking for boolean fields to satisfy reviewer feedback. - Maintain core security fix for non-object JSON values. Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
rdtect
deleted the
security-fix-load-persisted-state-validation-13195613228722407853
branch
The vulnerability fixed is the missing type checking on JSON parsed from local storage in
src/lib/core/persistence.ts. If the stored value is a non-object (e.g.,null,[], or a primitive), subsequent property access or logic might fail or lead to unexpected behavior.The fix involves:
if (!parsed || typeof parsed !== 'object' || Array.isArray(parsed))immediately afterJSON.parse.showBuildings,showClouds,syncToRealTime) to ensure they are of the correct type.The potential impact if left unfixed includes crashes if the browser storage is corrupted or tampered with, or unexpected UI behavior if flags are set to non-boolean values. The fix ensures the application remains robust and handles invalid data gracefully.
PR created automatically by Jules for task 13195613228722407853 started by @rdtect