Studio: harden auto permission network gates#7180
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request restricts low-level network modules (such as httpcore, boto3, and botocore) in sandboxed Python environments and expands static analysis regexes to detect additional HTML attributes, setAttribute calls, and bracket-access obfuscation on global host objects. The review feedback points out that the updated JavaScript bracket-access regex lacks word boundaries, which can lead to false positives on benign properties like window['is_open'] or window['fetching']. A code suggestion is provided to add word boundaries around the API names to resolve this issue.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| r"\b(?:window|self|globalThis|top|parent|frames)\s*\[[^\]]*" | ||
| r"(?:fetch|open|XMLHttpRequest|WebSocket|EventSource|importScripts|" | ||
| r"sendBeacon|serviceWorker)[^\]]*\]|" |
There was a problem hiding this comment.
The current regular expression for matching bracket access on global host objects uses [^\]]* around the API names (like fetch or open) without word boundaries. This causes severe false positives on benign properties or variables that contain these substrings (for example, window['is_open'], window['open_modal'], or window['fetching']), which are extremely common in frontend canvas code. Adding word boundaries (\b) around the API names prevents these false positives while still correctly catching obfuscated or direct bracket access.
| r"\b(?:window|self|globalThis|top|parent|frames)\s*\[[^\]]*" | |
| r"(?:fetch|open|XMLHttpRequest|WebSocket|EventSource|importScripts|" | |
| r"sendBeacon|serviceWorker)[^\]]*\]|" | |
| r"\b(?:window|self|globalThis|top|parent|frames)\s*\[[^\]]*" | |
| r"\b(?:fetch|open|XMLHttpRequest|WebSocket|EventSource|importScripts|" | |
| r"sendBeacon|serviceWorker)\b[^\]]*\]|" |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eed25b3845
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| def visit_Import(self, node): | ||
| for alias in node.names: | ||
| self._block_low_level_network_module(alias.name, node) |
There was a problem hiding this comment.
For snippets that dynamically import one of these clients, the sandbox check still returns safe because the new block only runs from visit_Import/visit_ImportFrom. For example, import importlib; boto3 = importlib.import_module('boto3'); boto3.client('s3').list_buckets() avoids the added import statement checks and boto3.client is not covered by _NETWORK_FQ_PREFIXES, so an installed low-level client can still make arbitrary network calls outside the host allowlist. Please also reject literal dynamic imports such as importlib.import_module(...) and __import__(...) for these module roots.
Useful? React with 👍 / 👎.
| r"\b(?:src|href|srcset|action|formaction|poster|data|ping)\s*=" | ||
| r"\s*[\"']?\s*(?:https?:|/)|" |
There was a problem hiding this comment.
When an anchor ping value starts with a bare relative URL, this pattern only checks the first token after = and misses later remote ping targets; e.g. <a ping='local https://evil/x'> remains auto-approved even though ping is a space-separated URL list and the browser will POST to the remote URL on click. Since this change adds ping to the network gate, the full attribute value needs to be scanned for any http(s): or root-relative entry rather than only the first URL.
Useful? React with 👍 / 👎.
| r"\b(?:window|self|globalThis|top|parent|frames)\s*\[[^\]]*" | ||
| r"(?:fetch|open|XMLHttpRequest|WebSocket|EventSource|importScripts|" | ||
| r"sendBeacon|serviceWorker)[^\]]*\]|" |
There was a problem hiding this comment.
For a canvas that invokes fetch through a split template key, such as window[`fet`+`ch`]('https://evil'), this new branch does not match because it only looks for a contiguous fetch/open token inside the brackets, and the computed-key rule below still only recognizes single/double quotes around +. Since this change now supports template-literal property access for the direct form, the same trivial split can still auto-run a networked canvas without approval.
Useful? React with 👍 / 👎.
| r"\.\s*setAttribute\s*\(\s*[\"'`](?:src|href|srcset|action|formaction|" | ||
| r"poster|data|ping)[\"'`]|" |
There was a problem hiding this comment.
For static canvases that build DOM nodes programmatically, e.g. <script>img.setAttribute('src','./local.png')</script>, this branch now asks for approval even though the value is only a relative local resource, because it checks only the attribute name and ignores the second argument. That regresses the documented behavior where static/relative-resource canvases auto-run; require the assigned value to be http(s) or root-relative before pausing.
Useful? React with 👍 / 👎.
| r"\b(?:src|href|srcset|action|formaction|poster|data|ping)\s*=" | ||
| r"\s*[\"']?\s*(?:https?:|/)|" |
There was a problem hiding this comment.
Because this regex runs over the whole HTML/JS string rather than only parsed attributes, adding data makes common static script state like <script>const data = '/tmp/file.json'</script> require approval even when nothing loads from the network. That regresses the static-canvas auto-run path for ordinary variables named data; constrain this branch to actual HTML attributes or otherwise avoid matching JavaScript assignments.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ec26768cbb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| def _is_import_loader(self, node) -> bool: | ||
| if isinstance(node, ast.Name): | ||
| return node.id in self.import_loader_aliases | ||
| if not isinstance(node, ast.Attribute) or not isinstance(node.value, ast.Name): | ||
| return False | ||
| if node.attr == "import_module": | ||
| return node.value.id in self.importlib_aliases | ||
| if node.attr == "__import__": | ||
| return node.value.id in self.builtins_aliases | ||
| return False |
There was a problem hiding this comment.
When sandboxed Python obtains the loader with a dynamic lookup, this check never sees an import loader because node.func is an ast.Call/ast.Subscript rather than a Name or Attribute. For example, import importlib; boto3 = getattr(importlib, "import_module")("boto3"); boto3.client("s3").list_buckets() still bypasses the new low-level module block, and boto3.client is not covered by the host allowlist scanner, so arbitrary network access can run after approval. Please treat literal getattr/namespace lookups of import_module or __import__ as import loaders too.
Useful? React with 👍 / 👎.
| _RENDER_HTML_SET_ATTRIBUTE_RE = re.compile( | ||
| r"""\.\s*setAttribute\s*\(\s* | ||
| (?P<quote>["'`]) | ||
| (?P<attr>src|href|srcset|action|formaction|poster|data|ping) | ||
| (?P=quote)\s*,\s*(?P<value>[^)]*)\)""", |
There was a problem hiding this comment.
For canvases that use optional chaining, such as <script>img.setAttribute?.('src','https://evil/x')</script>, this pattern does not match because it requires setAttribute( immediately after optional whitespace. Since the generic URL regex does not flag a bare https:// string, that canvas remains auto-approved even though the browser still sets the network-loading attribute when setAttribute exists. Please allow the optional-call token (?.) in this sink check.
Useful? React with 👍 / 👎.
| def visit_Call(self, node): | ||
| if node.args and self._is_import_loader(node.func): | ||
| module_name = self._static_string(node.args[0]) | ||
| if module_name is not None: | ||
| self._block_low_level_network_module(module_name, node) |
There was a problem hiding this comment.
When the dynamic import loader is called with the name keyword, this block is skipped because it only inspects positional node.args. For example, import importlib; boto3 = importlib.import_module(name='boto3'); boto3.client('s3').list_buckets() is not rejected by the sandbox, and boto3.client is still outside _NETWORK_FQ_PREFIXES, so the low-level client can make arbitrary network calls after approval. Please inspect the literal name= keyword for these import loaders as well.
Useful? React with 👍 / 👎.
| _RENDER_HTML_GLOBAL_BRACKET_RE = re.compile( | ||
| r"\b(?:window|self|globalThis|top|parent|frames)\s*\[([^\]]*)\]", | ||
| re.IGNORECASE | re.DOTALL, |
There was a problem hiding this comment.
For canvases that optional-chain the global before a computed key, such as <script>window?.['fetch']('https://evil/x')</script>, this regex no longer sees the window[...] access because of the intervening ?., and the generic fetch( pattern does not match fetch'](. That means a networked canvas can still auto-run without approval; please allow the optional-chain token before the bracket for the global hosts scanned here.
Useful? React with 👍 / 👎.
| value = _static_js_string(match.group("value")) | ||
| if value is None: | ||
| return True | ||
| if value.lstrip().lower().startswith(("http:", "https:", "/")): |
There was a problem hiding this comment.
When a programmatic setAttribute writes a URL-list attribute whose first entry is local, this check approves because it only tests the start of the static value. For example, <script>img.setAttribute('srcset','local.png 1x, https://evil/x.png 2x')</script> can still load the remote candidate, and ping has the same space-separated URL-list shape. Please scan the full value for remote or root-relative entries for URL-list attributes.
Useful? React with 👍 / 👎.
| (?P<quote>["'`]) | ||
| (?P<attr>src|href|srcset|action|formaction|poster|data|ping) | ||
| (?P=quote)\s*,\s*(?P<value>[^)]*)\)""", |
There was a problem hiding this comment.
When the attribute name is built from literal pieces, e.g. <script>img.setAttribute('s'+'rc','https://evil/x.png')</script>, this regex does not match because it requires a single quoted attribute token. The generic URL scanner does not flag a bare https:// string, so the canvas auto-runs even though the browser assigns a network-loading src. Please fold static attribute-name expressions before treating the call as safe.
Useful? React with 👍 / 👎.
| _RENDER_HTML_GLOBAL_BRACKET_RE = re.compile( | ||
| r"\b(?:window|self|globalThis|top|parent|frames)\s*\[([^\]]*)\]", | ||
| re.IGNORECASE | re.DOTALL, |
There was a problem hiding this comment.
In a classic canvas <script>, top-level this is window, but the new host-object regex only matches named globals. Thus <script>this['fetch']('https://evil/x')</script> is now auto-approved because the direct fetch( pattern does not see the quoted property, although it issues the same network request. Please include this or retain the literal bracket-member detection for this case.
Useful? React with 👍 / 👎.
1 participant
Summary
setAttribute, template property access, and computed global propertiesWhy
This is a follow-up to #7079.
Installed clients such as
httpcore,boto3, andbotocorecan open arbitrary endpoints without passing through the existingrequestsandurllibhost checks. Canvas detection also missed equivalent network-capable forms such aswindow[`fetch`],setAttribute("src", ...), and remote form actions.The sandbox now fails closed for those low-level clients, and Approve for me pauses before either the Python or canvas paths can make an unreviewed network request.
Validation
pytest studio/backend/tests/test_permission_mode.py studio/backend/tests/test_sandbox_tools.py -q -k 'not permission_mode_confirm_derivation and not confirm_gate_needs_stream'