From 0ceaca5f15a2cc7a3a621559c09857685204fbae Mon Sep 17 00:00:00 2001 From: Emin Deniz Date: Thu, 6 Aug 2026 22:54:23 +0300 Subject: [PATCH 1/9] Add a parser for GitHub source URLs, including /tree/, /blob/ and /commit/ forms MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The import dialog and the /ide/import/github/... deep links receive many shapes of GitHub reference: bare user/repo, http(s)/www/git@/git:// URLs (host case-insensitive, percent-encoding tolerated), .git suffixes, and web URLs carrying a branch and subdirectory (/tree//, /blob//, /commit/, and the self-qualified refs/heads|tags/ spellings GitHub's Raw button emits) — the /tree/ shape is what the Pebble appstore's "Remix on CloudPebble" button generates, and what StackBlitz, CodeSandbox, degit and create-next-app all accept. The module is dependency-free so its tests run with plain unittest. Branch names may contain slashes, so the ref-vs-path split is left to split_ref_and_path() against a real ref list, longest prefix first, the way gitpick and gitingest resolve GitHub web URLs — git's ref directory/file-conflict rule guarantees at most one branch and one tag can match, so the split is deterministic. Co-Authored-By: Claude Fable 5 --- cloudpebble/ide/tests/test_github_urls.py | 135 ++++++++++++++++++++++ cloudpebble/ide/utils/github_urls.py | 133 +++++++++++++++++++++ 2 files changed, 268 insertions(+) create mode 100644 cloudpebble/ide/tests/test_github_urls.py create mode 100644 cloudpebble/ide/utils/github_urls.py diff --git a/cloudpebble/ide/tests/test_github_urls.py b/cloudpebble/ide/tests/test_github_urls.py new file mode 100644 index 0000000..9981ec0 --- /dev/null +++ b/cloudpebble/ide/tests/test_github_urls.py @@ -0,0 +1,135 @@ +""" Tests for ide.utils.github_urls — the GitHub source parser behind the +import dialog and the /ide/import/github/... deep links (including the +appstore's "Remix on CloudPebble" button URLs). Dependency-free on purpose: + python -m unittest ide.tests.test_github_urls +""" + +from unittest import TestCase + +from ide.utils.github_urls import parse_github_source, split_ref_and_path, normalize_subpath + + +class TestParseGithubSource(TestCase): + def assert_parsed(self, source, user, project, kind=None, refpath=None): + parsed = parse_github_source(source) + self.assertIsNotNone(parsed, "expected %r to parse" % source) + self.assertEqual( + (user, project, kind, refpath), + (parsed.user, parsed.project, parsed.kind, parsed.refpath), + ) + + def test_bare_shorthand(self): + self.assert_parsed('Katharine/pebble-stopwatch', 'Katharine', 'pebble-stopwatch') + + def test_plain_domain(self): + self.assert_parsed('github.com/user/repo', 'user', 'repo') + + def test_https_www_git_suffix_trailing_slash(self): + self.assert_parsed('https://www.github.com/user/repo.git/', 'user', 'repo') + + def test_ssh_form(self): + self.assert_parsed('git@github.com:user/repo.git', 'user', 'repo') + + def test_git_protocol(self): + self.assert_parsed('git://github.com/user/repo', 'user', 'repo') + + def test_dotted_names(self): + self.assert_parsed('github.com/user.name/repo.js', 'user.name', 'repo.js') + + def test_tree_with_branch(self): + self.assert_parsed('https://github.com/user/repo/tree/main', 'user', 'repo', 'tree', 'main') + + def test_tree_with_branch_and_subpath(self): + # The appstore "Remix" button shape. + self.assert_parsed( + 'github.com/emindeniz99/pebble-signals/tree/main/faces/slothvec', + 'emindeniz99', 'pebble-signals', 'tree', 'main/faces/slothvec') + + def test_blob_form(self): + self.assert_parsed( + 'https://github.com/user/repo/blob/main/src/app.js', + 'user', 'repo', 'blob', 'main/src/app.js') + + def test_commit_form(self): + self.assert_parsed('github.com/user/repo/commit/abc123', 'user', 'repo', 'commit', 'abc123') + + def test_query_and_fragment_stripped(self): + self.assert_parsed( + 'https://github.com/user/repo/tree/main/dir?tab=readme#l10', + 'user', 'repo', 'tree', 'main/dir') + + def test_empty_tree_tail_is_plain_repo(self): + self.assert_parsed('github.com/user/repo/tree/', 'user', 'repo') + + def test_rejects_other_github_pages(self): + for source in ('github.com/user/repo/releases', + 'github.com/user/repo/issues/12', + 'github.com/user/repo/pull/3', + 'github.com/user/repo/wiki'): + self.assertIsNone(parse_github_source(source), source) + + def test_case_insensitive_host(self): + self.assert_parsed('GitHub.com/user/repo', 'user', 'repo') + self.assert_parsed('HTTPS://WWW.GITHUB.COM/user/repo', 'user', 'repo') + + def test_percent_encoded_input(self): + self.assert_parsed('github.com/user/repo/tree/main/my%20dir', + 'user', 'repo', 'tree', 'main/my dir') + + def test_rejects_non_github(self): + for source in ('gitlab.com/user/repo', 'https://example.com/user/repo', 'user', ''): + self.assertIsNone(parse_github_source(source), source) + + +class TestSplitRefAndPath(TestCase): + REFS = ['main', 'develop', 'feature/foo', 'feature/foo/bar', 'v1.0.0'] + + def test_plain_ref(self): + self.assertEqual(('main', ''), split_ref_and_path('main', self.REFS)) + + def test_ref_and_path(self): + self.assertEqual(('main', 'faces/slothvec'), + split_ref_and_path('main/faces/slothvec', self.REFS)) + + def test_slashed_ref_wins_over_short_ref(self): + self.assertEqual(('feature/foo', 'src'), + split_ref_and_path('feature/foo/src', self.REFS)) + + def test_longest_ref_wins(self): + self.assertEqual(('feature/foo/bar', 'src'), + split_ref_and_path('feature/foo/bar/src', self.REFS)) + + def test_tag_ref(self): + self.assertEqual(('v1.0.0', 'dir'), split_ref_and_path('v1.0.0/dir', self.REFS)) + + def test_unknown_ref_falls_back_to_first_segment(self): + self.assertEqual(('sha123', 'a/b'), split_ref_and_path('sha123/a/b', self.REFS)) + + def test_refs_heads_spelling(self): + self.assertEqual(('feature/foo', 'src'), + split_ref_and_path('refs/heads/feature/foo/src', self.REFS)) + + def test_refs_tags_spelling(self): + self.assertEqual(('v1.0.0', 'dir'), + split_ref_and_path('refs/tags/v1.0.0/dir', self.REFS)) + + def test_no_ref_list_falls_back_to_first_segment(self): + self.assertEqual(('main', 'faces/slothvec'), + split_ref_and_path('main/faces/slothvec', None)) + + +class TestNormalizeSubpath(TestCase): + def test_empty_and_root(self): + self.assertEqual('', normalize_subpath('')) + self.assertEqual('', normalize_subpath(None)) + self.assertEqual('', normalize_subpath('/')) + self.assertEqual('', normalize_subpath('.')) + + def test_collapses(self): + self.assertEqual('faces/slothvec', normalize_subpath('/faces//slothvec/')) + self.assertEqual('a/b', normalize_subpath('a/./b')) + + def test_rejects_escapes(self): + for bad in ('..', '../x', 'a/../../b'): + with self.assertRaises(ValueError): + normalize_subpath(bad) diff --git a/cloudpebble/ide/utils/github_urls.py b/cloudpebble/ide/utils/github_urls.py new file mode 100644 index 0000000..6ac9acc --- /dev/null +++ b/cloudpebble/ide/utils/github_urls.py @@ -0,0 +1,133 @@ +""" Parsing for GitHub project "sources" — the strings users paste into the +import dialog or arrive with via /ide/import/github/... deep links (e.g. the +appstore's "Remix on CloudPebble" button, which links to +.../import/github///tree//). + +This module is deliberately dependency-free (no Django, no PyGithub) so it can +be unit-tested standalone. Anything that needs the network — resolving which +prefix of an ambiguous "/" remainder is a real branch — lives in +ide.tasks.git and uses split_ref_and_path() from here with a real ref list. +""" +import posixpath +import re +from collections import namedtuple +from urllib.parse import unquote + +__author__ = 'emindeniz99' + +# A parsed source: +# user, project: the repository coordinates. +# kind: None (plain repo), 'tree', 'blob' or 'commit'. +# refpath: the raw remainder after /tree/ or /blob/ — "" or +# "/". GitHub branch names may contain slashes, so the split +# between ref and path can only be decided against the repository's real +# ref names (split_ref_and_path below). +GithubSource = namedtuple('GithubSource', ['user', 'project', 'kind', 'refpath']) + +# Accepted forms (query strings and fragments are ignored): +# user/repo +# github.com/user/repo[.git][/] (bare, http, https, www) +# git@github.com:user/repo[.git] +# git://github.com/user/repo[.git] +# .../user/repo/tree/[/] +# .../user/repo/blob// +# .../user/repo/commit/ +# Anything else after the repo (releases, issues, wiki, ...) is rejected so a +# nonsensical import fails loudly instead of silently importing the repo root. +_SOURCE_RE = re.compile(r""" + ^ + (?: + (?i:(?:https?://)?(?:www\.)?github\.com)/ | + (?i:git@github\.com): | + (?i:git://github\.com)/ + )? + (?P[\w.-]+) + / + (?P[\w.-]+?) + (?:\.git)? + (?: + / + (?: + (?Ptree|blob|commit) + (?: + / + (?P[^?\#]+?) + )? + )? + )? + /? + (?:[?\#].*)? + $ +""", re.VERBOSE) + + +def parse_github_source(source): + """ Parse a GitHub repository source string. + :param source: whatever the user gave us (URL, shorthand, deep-link tail). + :return: a GithubSource, or None if this isn't a recognizable GitHub source. + """ + match = _SOURCE_RE.match(unquote(source.strip())) + if match is None: + return None + kind = match.group('kind') + refpath = match.group('refpath') + if refpath is not None: + refpath = refpath.strip('/') + if not refpath: + # "…/repo/tree/" and "…/repo/tree" carry no information — treat as the + # plain repository rather than rejecting the whole import. + kind, refpath = None, None + return GithubSource(match.group('user'), match.group('project'), kind, refpath) + + +def split_ref_and_path(refpath, ref_names): + """ Split a /tree/ or /blob/ remainder into (ref, path). + + GitHub branch names may contain slashes ("feature/foo"), so "a/b/c" is + ambiguous between ref "a/b" + path "c" and ref "a" + path "b/c". We match + the remainder against the repository's REAL ref names, longest prefix + first (the approach used by gitpick and gitingest; create-next-app instead + naive-splits and asks the user for --example-path in the ambiguous case). + Git's directory/file conflict rule means at most ONE branch and at most + ONE tag can prefix-match, so the split is deterministic — longest-first + only decides the cross-namespace case (tag "release" vs branch + "release/1.0") in favor of the more specific ref. + + GitHub's self-qualified spellings ("refs/heads//..." and + "refs/tags//...", which its Raw button emits) are recognized: the + refs/... prefix is stripped and the remainder matched as usual. + + :param refpath: "" or "/" (already stripped of slashes). + :param ref_names: iterable of the repository's branch and tag names. Pass + None when the list is unavailable — the first segment is then taken as + the ref, which is correct for every ref without a slash in its name + (commit SHAs included, since a SHA is never slashed). + :return: (ref, path) — path is '' when the remainder is just a ref. + """ + parts = refpath.split('/') + if len(parts) >= 3 and parts[0] == 'refs' and parts[1] in ('heads', 'tags'): + parts = parts[2:] + refpath = '/'.join(parts) + if len(parts) == 1: + return refpath, '' + if ref_names is not None: + names = set(ref_names) + for i in range(len(parts), 0, -1): + candidate = '/'.join(parts[:i]) + if candidate in names: + return candidate, '/'.join(parts[i:]) + return parts[0], '/'.join(parts[1:]) + + +def normalize_subpath(path): + """ Normalize a repo-relative subdirectory: collapse separators, forbid + escapes. Returns '' for the repo root; raises ValueError on '..' or + absolute paths (deep links are attacker-suppliable). """ + if not path: + return '' + normalized = posixpath.normpath(path.strip('/')) + if normalized in ('.', ''): + return '' + if normalized.startswith(('/', '..')): + raise ValueError("Invalid project path: %r" % path) + return normalized From e2c88756e6db9954fc6c63c2f8e886d927103bdf Mon Sep 17 00:00:00 2001 From: Emin Deniz Date: Thu, 6 Aug 2026 22:54:23 +0300 Subject: [PATCH 2/9] Let archive imports pin the project to a subdirectory (root_hint) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit find_project_root_and_manifest() picks the first valid manifest in the archive, which is wrong for repositories that contain several projects (a library whose root is itself a Pebble project, plus examples and watchfaces in subdirectories). A root_hint restricts the match to the named directory — allowing at most one wrapping folder above it, since GitHub archives prefix everything with -/ — and failure reports the path it looked at. A bare suffix match is deliberately NOT enough (a hint like 'src' must not latch onto any directory of that name at any depth); tests cover both zip shapes and the rejections. Co-Authored-By: Claude Fable 5 --- cloudpebble/ide/tasks/archive.py | 4 +- .../ide/tests/test_find_project_root.py | 50 +++++++++++++++++++ cloudpebble/ide/utils/project.py | 27 +++++++++- 3 files changed, 78 insertions(+), 3 deletions(-) diff --git a/cloudpebble/ide/tasks/archive.py b/cloudpebble/ide/tasks/archive.py index 3f1fe0b..5c6c5c7 100644 --- a/cloudpebble/ide/tasks/archive.py +++ b/cloudpebble/ide/tasks/archive.py @@ -145,7 +145,7 @@ def ends_with_any(s, options): @shared_task(acks_late=True) -def do_import_archive(project_id, archive, delete_project=False, wipe_existing=False): +def do_import_archive(project_id, archive, delete_project=False, wipe_existing=False, root_hint=None): project = Project.objects.get(pk=project_id) try: with tempfile.NamedTemporaryFile(suffix='.zip') as archive_file: @@ -172,7 +172,7 @@ def do_import_archive(project_id, archive, delete_project=False, wipe_existing=F raise InvalidProjectArchiveException("Too many files in zip file.") archive_items = [ArchiveProjectItem(z, x) for x in contents] - base_dir, manifest_item = find_project_root_and_manifest(archive_items) + base_dir, manifest_item = find_project_root_and_manifest(archive_items, root_hint=root_hint) dir_end = len(base_dir) def make_valid_filename(zip_entry): diff --git a/cloudpebble/ide/tests/test_find_project_root.py b/cloudpebble/ide/tests/test_find_project_root.py index fd2fc67..c8f8969 100644 --- a/cloudpebble/ide/tests/test_find_project_root.py +++ b/cloudpebble/ide/tests/test_find_project_root.py @@ -149,3 +149,53 @@ def test_skip_build_and_node_modules_together(self): "project/src/c/main.c", "project/resources/fonts/font.ttf", ], "project/", "project/package.json") + + +class TestFindProjectRootWithHint(TestCase): + """ root_hint pins the search to one subdirectory — the /tree// + import case, where the repository may contain several projects. """ + + REPO = [ + # A repository that is ALSO a valid project at its root (a library + # shipping its own demo), plus two nested projects — the shape that + # made unhinted imports pick the wrong one. + "repo-main/package.json", + "repo-main/src/", + "repo-main/faces/slothvec/package.json", + "repo-main/faces/slothvec/src/", + "repo-main/examples/other/package.json", + ] + + def find(self, contents, root_hint): + return find_project_root_and_manifest( + [FakeProjectItem(item) for item in contents], root_hint=root_hint) + + def test_hint_picks_nested_project_over_root(self): + base_dir, manifest = self.find(self.REPO, "faces/slothvec") + self.assertEqual(base_dir, "repo-main/faces/slothvec/") + self.assertEqual(manifest.name, "repo-main/faces/slothvec/package.json") + + def test_hint_matches_archive_without_wrapper_dir(self): + base_dir, manifest = self.find( + ["faces/slothvec/package.json", "faces/slothvec/src/"], "faces/slothvec") + self.assertEqual(base_dir, "faces/slothvec/") + + def test_hint_with_no_project_there_throws(self): + with self.assertRaises(InvalidProjectArchiveException): + self.find(self.REPO, "faces/missing") + + def test_hint_does_not_match_deeper_manifests(self): + with self.assertRaises(InvalidProjectArchiveException): + self.find(self.REPO, "faces") + + def test_hint_suffix_alone_is_not_enough(self): + # 'slothvec' names the right LEAF, but the project lives at + # faces/slothvec — a lazy suffix match would import it anyway and + # mask a wrong ref/path split upstream. It must fail loudly instead. + with self.assertRaises(InvalidProjectArchiveException): + self.find(self.REPO, "slothvec") + + def test_no_hint_keeps_existing_behavior(self): + base_dir, manifest = find_project_root_and_manifest( + [FakeProjectItem(item) for item in self.REPO]) + self.assertEqual(base_dir, "repo-main/") diff --git a/cloudpebble/ide/utils/project.py b/cloudpebble/ide/utils/project.py index d0f2a10..dcbedf2 100644 --- a/cloudpebble/ide/utils/project.py +++ b/cloudpebble/ide/utils/project.py @@ -49,9 +49,26 @@ def is_manifest(kind, contents): return False -def find_project_root_and_manifest(project_items): +def _dir_matches_hint(base_dir, root_hint): + """ True if base_dir ('' or 'a/b/') is the root_hint directory, allowing + at most ONE wrapping folder above it ('-/...' in GitHub zips). + The wrapper limit matters: a bare suffix match would let a hint like + 'src' latch onto any directory of that name at any depth. """ + stripped = base_dir.rstrip('/') + if stripped == root_hint: + return True + suffix = '/' + root_hint + return stripped.endswith(suffix) and '/' not in stripped[:-len(suffix)] + + +def find_project_root_and_manifest(project_items, root_hint=None): """ Given the contents of an archive, find a valid Pebble project. :param project_items: A list of BaseProjectItems + :param root_hint: If given, only accept a project whose directory is + root_hint, relative to the archive's own root (archives from GitHub + wrap everything in a '-/' folder, so the hint is matched + against the tail of the directory). Used by /tree// + imports to pick one project out of a repository that contains several. :return: A tuple of (path_to_project, manifest BaseProjectItem) """ SRC_DIR = 'src/' @@ -84,6 +101,11 @@ def find_project_root_and_manifest(project_items): # The base dir is the location of the manifest file without the manifest filename. base_dir = base_dir[:dir_end] + # A root hint pins the project to one directory; manifests anywhere + # else (e.g. the repository root) don't count. + if root_hint and not _dir_matches_hint(base_dir, root_hint): + continue + # If we found a valid package.json, just return. if name == PACKAGE_MANIFEST: return base_dir, manifest_item @@ -105,6 +127,9 @@ def find_project_root_and_manifest(project_items): return base_dir, manifest_item # If we didn't find a valid project but we did find a broken manifest file, complain about it specifically. + if root_hint: + raise InvalidProjectArchiveException( + _("No valid Pebble project found at '%s' in this repository." % root_hint)) if invalid_package_path: raise InvalidProjectArchiveException(_("The file %s does not contain a valid JSON object." % invalid_package_path)) else: From 0d9997961dbe3f452c77bd3f50e3616a18b26f75 Mon Sep 17 00:00:00 2001 From: Emin Deniz Date: Thu, 6 Aug 2026 22:54:23 +0300 Subject: [PATCH 3/9] Support GitHub web URLs with branch and subdirectory in project import MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Importing github.com///tree// now does what it says: the API parses the URL (previously the regex silently dropped everything after the repo and imported the wrong project from the default branch), the celery task resolves the ambiguous / remainder against the repository's real branch and tag names — falling back to probing the strict codeload zip/refs/heads/... endpoint when the API is unavailable, since bare archive/.zip answers 200 even for junk refs (measured: archive/main/anything.zip serves main) — and the archive importer pins the project root to the subdirectory. /blob// imports the file's directory; /commit/ imports at that commit. Also fixes the empty-branch default: the client used to hardcode 'master', which broke every main-default repository; an empty branch now imports codeload's HEAD, i.e. the repository's default branch. add_remote is rejected for subdirectory imports (a linked push would target the repository root) with a clear message. Legacy /ide/import/github/// deep links keep working. This makes the Pebble appstore's "Remix on CloudPebble" button work: its links have the /ide/import/github///tree// shape, which previously prefilled 'tree/main/...' into the branch box. Co-Authored-By: Claude Fable 5 --- cloudpebble/ide/api/project.py | 26 +++++-- cloudpebble/ide/static/ide/js/project_list.js | 26 +++++-- cloudpebble/ide/tasks/git.py | 74 ++++++++++++++++++- cloudpebble/ide/views/run.py | 2 +- 4 files changed, 109 insertions(+), 19 deletions(-) diff --git a/cloudpebble/ide/api/project.py b/cloudpebble/ide/api/project.py index feda84d..1e74bfa 100644 --- a/cloudpebble/ide/api/project.py +++ b/cloudpebble/ide/api/project.py @@ -20,6 +20,7 @@ from ide.tasks.build import run_compile from ide.tasks.gist import import_gist from ide.tasks.git import do_import_github +from ide.utils.github_urls import parse_github_source from ide.utils.alloy_templates import list_alloy_templates, build_template_archive from ide.utils.c_templates import list_c_templates, build_c_template_archive from utils.td_helper import send_td_event @@ -1029,14 +1030,24 @@ def import_zip(request): def import_github(request): name = request.POST['name'] repo = request.POST['repo'] - branch = request.POST['branch'] + branch = request.POST.get('branch', '') add_remote = (request.POST['add_remote'] == 'true') - match = re.match(r'^(?:https?://|git@|git://)?(?:www\.)?github\.com[/:]([\w.-]+)/([\w.-]+?)(?:\.git|/|$)', repo) - if match is None: + source = parse_github_source(repo) + if source is None: raise BadRequest(_("Invalid Github URL.")) - github_user = match.group(1) - github_project = match.group(2) + github_user = source.user + github_project = source.project + if source.refpath: + # A /tree/, /blob/ or /commit/ URL is authoritative for both the ref + # and the subdirectory (the celery task resolves the ambiguous split + # against the repository's real refs); a separately-typed branch would + # conflict with it, so it is ignored. + branch = '' + if add_remote: + raise BadRequest(_("Linking a repository is not supported for /tree/ or /blob/ " + "imports — import the project first, then add the remote from " + "its settings.")) try: project = Project.objects.create(owner=request.user, name=name) @@ -1045,10 +1056,11 @@ def import_github(request): if add_remote: project.github_repo = "%s/%s" % (github_user, github_project) - project.github_branch = branch + project.github_branch = branch or None project.save() - task = do_import_github.delay(project.id, github_user, github_project, branch, delete_project=True) + task = do_import_github.delay(project.id, github_user, github_project, branch, delete_project=True, + github_refpath=source.refpath, github_kind=source.kind) return {'task_id': task.task_id, 'project_id': project.id} diff --git a/cloudpebble/ide/static/ide/js/project_list.js b/cloudpebble/ide/static/ide/js/project_list.js index 564b9ab..a930629 100644 --- a/cloudpebble/ide/static/ide/js/project_list.js +++ b/cloudpebble/ide/static/ide/js/project_list.js @@ -178,14 +178,14 @@ $(function() { active_set.find('.errors').removeClass('hide').text(gettext("You must specify a project name.")); return; } - // This is identical to the regex used on the server. + // A prefix check only — the server (ide.utils.github_urls) is the + // authority and also understands /tree// and /blob/ URLs. if(!/^(?:https?:\/\/|git@|git:\/\/)?(?:www\.)?github\.com[\/:]([\w.-]+)\/([\w.-]+?)(?:\.git|\/|$)/.test(url)) { active_set.find('.errors').removeClass('hide').text(gettext("You must specify a complete GitHub project URL")); return; } - if(branch.length == 0) { - branch = 'master'; - } + // An empty branch imports the repository's default branch (the old + // hardcoded 'master' fallback broke every main-default repository). disable_import_controls(); active_set.find('.progress').removeClass('hide'); do_import(Ajax.Post('/ide/import/github', { @@ -227,10 +227,20 @@ $(function() { if (path.indexOf('/ide/import/github/') === 0) { var parts = path.substr(1).split('/'); $('#import-prompt').modal(); - $('#import-github-name').val(parts[3]); - $('#import-github-url').val('github.com/' + parts[3] + '/' + parts[4]); - if (parts.length > 5) { - $('#import-github-branch').val(parts.slice(5).join('/')); + var tail = parts.slice(5).filter(function(p) { return p.length > 0; }); + if (tail.length > 1 && (tail[0] == 'tree' || tail[0] == 'blob' || tail[0] == 'commit')) { + // A GitHub web URL shape (e.g. the appstore's "Remix" button: + // .../import/github///tree//). Hand + // the whole thing to the server, which resolves branch vs + // subdirectory against the repository's real refs. + $('#import-github-name').val(tail.length > 2 ? tail[tail.length - 1] : parts[4]); + $('#import-github-url').val('github.com/' + parts.slice(3).concat([]).join('/')); + } else { + $('#import-github-name').val(parts[3]); + $('#import-github-url').val('github.com/' + parts[3] + '/' + parts[4]); + if (tail.length) { + $('#import-github-branch').val(tail.join('/')); + } } $('a[href=#import-github]').tab('show'); } diff --git a/cloudpebble/ide/tasks/git.py b/cloudpebble/ide/tasks/git.py index dfd0d4a..825069b 100644 --- a/cloudpebble/ide/tasks/git.py +++ b/cloudpebble/ide/tasks/git.py @@ -1,5 +1,6 @@ import base64 import io +import posixpath from concurrent.futures import ThreadPoolExecutor from io import BytesIO from urllib.request import urlopen, Request @@ -22,6 +23,7 @@ from ide.tasks import do_import_archive, run_compile from ide.tasks.archive import get_filename_variant from ide.utils.git import git_sha, git_blob +from ide.utils.github_urls import split_ref_and_path, normalize_subpath from ide.utils.project import find_project_root_and_manifest, BaseProjectItem, InvalidProjectArchiveException, MANIFEST_KINDS from ide.utils.sdk import generate_manifest_dict, generate_manifest, generate_wscript_file, load_manifest_dict, manifest_name_for_project from utils.td_helper import send_td_event @@ -40,7 +42,8 @@ def exception_reason(error): @shared_task(acks_late=True) -def do_import_github(project_id, github_user, github_project, github_branch, delete_project=False): +def do_import_github(project_id, github_user, github_project, github_branch, delete_project=False, + github_refpath=None, github_kind=None): project = None user = None try: @@ -50,6 +53,18 @@ def do_import_github(project_id, github_user, github_project, github_branch, del except: pass + github_path = '' + if github_refpath: + # A /tree/ or /blob/ URL: the branch and the subdirectory are one + # ambiguous string until matched against the repository's refs. + github_branch, github_path = resolve_ref_and_path( + user, github_user, github_project, github_refpath, github_kind) + if not github_branch: + # codeload resolves HEAD to the repository's default branch, so an + # empty branch imports what visitors see — instead of the old + # hardcoded 'master', which broke every main-default repository. + github_branch = 'HEAD' + url = "https://github.com/%s/%s/archive/%s.zip" % (github_user, github_project, github_branch) auth_url = get_authenticated_archive_url(user, github_user, github_project, github_branch) archive = None @@ -68,7 +83,7 @@ def do_import_github(project_id, github_user, github_project, github_branch, del % (github_user, github_project, github_branch) ) - return do_import_archive(project_id, archive.read()) + return do_import_archive(project_id, archive.read(), root_hint=github_path or None) except Exception as e: if delete_project and project is not None: try: @@ -80,12 +95,65 @@ def do_import_github(project_id, github_user, github_project, github_branch, del 'reason': exception_reason(e), 'github_user': github_user, 'github_project': github_project, - 'github_branch': github_branch + 'github_branch': github_branch, + 'github_refpath': github_refpath } }, user=user) raise +def resolve_ref_and_path(user, github_user, github_project, refpath, kind): + """ Resolve a /tree/ or /blob/ remainder ("[/]") into a real + (ref, subdirectory) pair. Branch names may contain slashes, so the split + is decided against the repository's actual refs — via the GitHub API when + possible, else by probing which archive actually exists (longest candidate + first), the way gitpick and gitingest resolve GitHub web URLs. """ + if kind == 'commit': + return refpath, '' + + ref, path = None, None + ref_names = get_ref_names(user, github_user, github_project) + if ref_names is not None: + ref, path = split_ref_and_path(refpath, ref_names) + elif '/' in refpath: + # No API access (rate limit, private repo without a token): probe the + # public archive endpoint. Longest first, so 'feature/foo/src' tries + # branch 'feature/foo' before branch 'feature'. The refs-qualified + # codeload form MUST be used here: bare archive/.zip answers 200 + # for junk like 'main/anything', while zip/refs/heads/ is strict + # (measured: refs/heads/main 200, refs/heads/main/faces 404). + parts = refpath.split('/') + start = len(parts) if kind == 'tree' else len(parts) - 1 + for i in range(start, 0, -1): + candidate = '/'.join(parts[:i]) + if any(file_exists("https://codeload.github.com/%s/%s/zip/refs/%s/%s" + % (github_user, github_project, refkind, candidate)) + for refkind in ('heads', 'tags')): + ref, path = candidate, '/'.join(parts[i:]) + break + if ref is None: + ref, path = split_ref_and_path(refpath, None) + + if kind == 'blob': + # A blob URL names a file; the project is the directory it lives in. + path = posixpath.dirname(path) + try: + path = normalize_subpath(path) + except ValueError: + raise Exception("Invalid project path in GitHub URL: '%s'" % refpath) + return ref, path + + +def get_ref_names(user, github_user, github_project): + """ Branch + tag names, or None when the API is unavailable. """ + try: + g = get_github(user) if user is not None else Github() + repo = g.get_repo("%s/%s" % (github_user, github_project)) + return [b.name for b in repo.get_branches()] + [t.name for t in repo.get_tags()] + except Exception: + return None + + def file_exists(url): request = Request(url) request.get_method = lambda: 'HEAD' diff --git a/cloudpebble/ide/views/run.py b/cloudpebble/ide/views/run.py index 686ae1e..8514d27 100644 --- a/cloudpebble/ide/views/run.py +++ b/cloudpebble/ide/views/run.py @@ -104,7 +104,7 @@ def run_app(request, app_id): github_import_url = '' if source and 'github.com/' in source: # Extract account/project from URL like https://github.com/user/repo - gh_match = re.search(r'github\.com/([^/]+/[^/]+)', source) + gh_match = re.search(r"github\.com/([\w.-]+/[\w.-]+(?:/(?:tree|blob|commit)/[^\s'\"?#]+)?)", source) if gh_match: github_import_url = '/ide/import/github/%s' % gh_match.group(1) From 9199fa57dead0a866b8f8a3ee3b7be6b8ad9078a Mon Sep 17 00:00:00 2001 From: Emin Deniz Date: Thu, 6 Aug 2026 22:59:41 +0300 Subject: [PATCH 4/9] Keep every legacy import form working, with tests on both sides MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The old server regex matched github.com[/:], so the bare-colon form github.com:user/repo imported fine — the new parser now accepts it too, and a parity test walks every form the old regex accepted. On the JS side, vitest tests pin the deep-link prefill contract: the legacy /ide/import/github//[/] links (slashed branches included) behave exactly as before, while /tree// links hand the whole URL to the server with the branch box left empty. Co-Authored-By: Claude Fable 5 --- .../__tests__/github-import-prefill.test.js | 97 +++++++++++++++++++ cloudpebble/ide/tests/test_github_urls.py | 18 ++++ cloudpebble/ide/utils/github_urls.py | 6 +- 3 files changed, 119 insertions(+), 2 deletions(-) create mode 100644 cloudpebble/ide/static/ide/js/__tests__/github-import-prefill.test.js diff --git a/cloudpebble/ide/static/ide/js/__tests__/github-import-prefill.test.js b/cloudpebble/ide/static/ide/js/__tests__/github-import-prefill.test.js new file mode 100644 index 0000000..d7367e7 --- /dev/null +++ b/cloudpebble/ide/static/ide/js/__tests__/github-import-prefill.test.js @@ -0,0 +1,97 @@ +import { describe, it, expect, vi } from 'vitest'; +import { readFileSync } from 'fs'; +import { resolve } from 'path'; + +// project_list.js is one $(function(){...}) block; the jQuery mock runs the +// callback immediately, so loading the file executes the deep-link prefill. +// Every jQuery method is a chainable no-op except val(), which records per +// selector — enough to assert what lands in the import dialog's fields. +function makeJqueryMock() { + var elements = {}; + var $ = function(selector) { + if (typeof selector === 'function') { + selector(); + return $; + } + if (!elements[selector]) { + var store = { value: undefined }; + var el = new Proxy(store, { + get: function(target, prop) { + if (prop === 'val') { + return function(v) { + if (v === undefined) return target.value; + target.value = v; + return el; + }; + } + if (prop === 'text') return function() { return ''; }; + if (prop === 'length') return 0; + if (prop === '_isMock') return true; + return function() { return el; }; + } + }); + elements[selector] = el; + } + return elements[selector]; + }; + $.Deferred = vi.fn(); + $.elements = elements; + return $; +} + +function loadWithPath(pathname) { + var code = readFileSync(resolve(__dirname, '..', 'project_list.js'), 'utf8'); + var $ = makeJqueryMock(); + var fn = new Function( + '$', 'jQuery', 'gettext', 'jquery_csrf_setup', 'ga', 'Ajax', 'CloudPebble', 'location', + code + ); + fn($, $, function(s) { return s; }, vi.fn(), vi.fn(), {}, {}, { pathname: pathname }); + return { + name: $('#import-github-name').val(), + url: $('#import-github-url').val(), + branch: $('#import-github-branch').val() + }; +} + +describe('GitHub import deep-link prefill', () => { + it('hands a /tree// URL (the appstore Remix shape) to the server whole', () => { + var fields = loadWithPath('/ide/import/github/emindeniz99/pebble-signals/tree/main/faces/slothvec'); + expect(fields.url).toBe('github.com/emindeniz99/pebble-signals/tree/main/faces/slothvec'); + expect(fields.name).toBe('slothvec'); + // The URL is authoritative for the ref — the branch box stays empty. + expect(fields.branch).toBeUndefined(); + }); + + it('suggests the repo name when a /tree/ URL has no subdirectory', () => { + var fields = loadWithPath('/ide/import/github/user/repo/tree/main'); + expect(fields.url).toBe('github.com/user/repo/tree/main'); + expect(fields.name).toBe('repo'); + expect(fields.branch).toBeUndefined(); + }); + + it('keeps the legacy /// deep-link contract', () => { + var fields = loadWithPath('/ide/import/github/user/repo/dev'); + expect(fields.url).toBe('github.com/user/repo'); + expect(fields.name).toBe('user'); + expect(fields.branch).toBe('dev'); + }); + + it('keeps legacy slashed-branch deep links', () => { + var fields = loadWithPath('/ide/import/github/user/repo/feat/x'); + expect(fields.url).toBe('github.com/user/repo'); + expect(fields.branch).toBe('feat/x'); + }); + + it('leaves the branch empty for a plain repo link (server imports the default branch)', () => { + var fields = loadWithPath('/ide/import/github/user/repo'); + expect(fields.url).toBe('github.com/user/repo'); + expect(fields.branch).toBeUndefined(); + }); + + it('does nothing outside the import deep link', () => { + var fields = loadWithPath('/ide/'); + expect(fields.url).toBeUndefined(); + expect(fields.name).toBeUndefined(); + }); +}); diff --git a/cloudpebble/ide/tests/test_github_urls.py b/cloudpebble/ide/tests/test_github_urls.py index 9981ec0..ea0cd62 100644 --- a/cloudpebble/ide/tests/test_github_urls.py +++ b/cloudpebble/ide/tests/test_github_urls.py @@ -76,6 +76,24 @@ def test_percent_encoded_input(self): self.assert_parsed('github.com/user/repo/tree/main/my%20dir', 'user', 'repo', 'tree', 'main/my dir') + def test_legacy_colon_form(self): + # The pre-parser server regex matched github.com[/:], so links using a + # bare colon after the domain must keep importing. + self.assert_parsed('github.com:user/repo', 'user', 'repo') + self.assert_parsed('https://github.com:user/repo.git', 'user', 'repo') + + def test_legacy_regex_parity(self): + # Every form the OLD import regex accepted (it truncated at the repo + # name) must still parse to the same user/project pair. + for source in ('https://github.com/user/repo', + 'http://github.com/user/repo', + 'www.github.com/user/repo', + 'git@github.com:user/repo', + 'git://github.com/user/repo', + 'github.com/user/repo.git', + 'github.com/user/repo/'): + self.assert_parsed(source, 'user', 'repo') + def test_rejects_non_github(self): for source in ('gitlab.com/user/repo', 'https://example.com/user/repo', 'user', ''): self.assertIsNone(parse_github_source(source), source) diff --git a/cloudpebble/ide/utils/github_urls.py b/cloudpebble/ide/utils/github_urls.py index 6ac9acc..ecde02b 100644 --- a/cloudpebble/ide/utils/github_urls.py +++ b/cloudpebble/ide/utils/github_urls.py @@ -26,7 +26,9 @@ # Accepted forms (query strings and fragments are ignored): # user/repo -# github.com/user/repo[.git][/] (bare, http, https, www) +# github.com/user/repo[.git][/] (bare, http, https, www; the +# legacy 'github.com:user/repo' +# colon form stays accepted) # git@github.com:user/repo[.git] # git://github.com/user/repo[.git] # .../user/repo/tree/[/] @@ -37,7 +39,7 @@ _SOURCE_RE = re.compile(r""" ^ (?: - (?i:(?:https?://)?(?:www\.)?github\.com)/ | + (?i:(?:https?://)?(?:www\.)?github\.com)[/:] | (?i:git@github\.com): | (?i:git://github\.com)/ )? From f7ca2f2f9c0bb92652839af0dd66e6b38114660d Mon Sep 17 00:00:00 2001 From: Emin Deniz Date: Fri, 7 Aug 2026 00:07:48 +0300 Subject: [PATCH 5/9] Address review feedback on the GitHub subpath import - Split raw query/fragment delimiters before percent-decoding, so an encoded '#' or '?' survives inside a path segment (and drop the regex's now-redundant fragment tail) - Strip refs/heads|tags qualifiers before the codeload probe fallback and pin the probe to that namespace - Keep branch-only /tree/ URLs on the full legacy flow, including "Use as Git remote" - Stop recommending linking-from-settings for subdirectory imports (a linked push would re-find a project root in the whole repository and could overwrite a different project); commit URLs get their own message - Apply the 400-file limit to the hinted project subtree instead of the whole repository, with a 50k ceiling on the pre-scan - Reject any '..' segment in subpaths outright - Route the build-status GitHub link through parse_github_source (one authority, case-insensitive host) with a plain-repository fallback, and percent-encode refs in generated URLs - Translate the hinted-import error before interpolating the hint - Suggest the repository name for blob/commit deep links (tree URLs keep the subdirectory suggestion) - Tests: resolver unit tests with a mocked ref list and probe, /ide/import/github linking flows, hinted big-repository archive imports, run-view link translation, parser delimiter cases (Python 237 -> 252, JS 34 -> 35) Co-Authored-By: Claude Fable 5 --- cloudpebble/ide/api/project.py | 28 ++-- .../__tests__/github-import-prefill.test.js | 9 ++ cloudpebble/ide/static/ide/js/project_list.js | 9 +- cloudpebble/ide/tasks/archive.py | 14 +- cloudpebble/ide/tasks/git.py | 19 ++- cloudpebble/ide/tests/test_github_import.py | 146 ++++++++++++++++++ cloudpebble/ide/tests/test_github_urls.py | 11 +- cloudpebble/ide/tests/test_import_archive.py | 54 +++++++ cloudpebble/ide/tests/test_run_github_link.py | 54 +++++++ cloudpebble/ide/utils/github_urls.py | 17 +- cloudpebble/ide/utils/project.py | 2 +- cloudpebble/ide/views/run.py | 37 ++++- 12 files changed, 371 insertions(+), 29 deletions(-) create mode 100644 cloudpebble/ide/tests/test_github_import.py create mode 100644 cloudpebble/ide/tests/test_run_github_link.py diff --git a/cloudpebble/ide/api/project.py b/cloudpebble/ide/api/project.py index 1e74bfa..7a6dc7c 100644 --- a/cloudpebble/ide/api/project.py +++ b/cloudpebble/ide/api/project.py @@ -1038,16 +1038,26 @@ def import_github(request): github_user = source.user github_project = source.project - if source.refpath: - # A /tree/, /blob/ or /commit/ URL is authoritative for both the ref - # and the subdirectory (the celery task resolves the ambiguous split - # against the repository's real refs); a separately-typed branch would - # conflict with it, so it is ignored. + refpath, kind = source.refpath, source.kind + if refpath and kind == 'tree' and '/' not in refpath: + # A slash-free /tree/ is unambiguously a branch or tag — exactly + # equivalent to typing it into the branch box, so every existing flow + # (add_remote included) stays available. + branch, refpath, kind = refpath, None, None + elif refpath: + # The URL is authoritative for both the ref and the subdirectory (the + # celery task resolves the ambiguous split against the repository's + # real refs); a separately-typed branch would conflict, so it is + # ignored. branch = '' if add_remote: - raise BadRequest(_("Linking a repository is not supported for /tree/ or /blob/ " - "imports — import the project first, then add the remote from " - "its settings.")) + # Deliberately NOT suggesting to link later from settings: the + # project model has no notion of a subdirectory, so a linked push + # would re-find a project root in the full repository tree and + # could overwrite a different project there. + if kind == 'commit': + raise BadRequest(_("Linking a repository is not supported for commit imports.")) + raise BadRequest(_("Linking a repository is not supported for subdirectory imports.")) try: project = Project.objects.create(owner=request.user, name=name) @@ -1060,7 +1070,7 @@ def import_github(request): project.save() task = do_import_github.delay(project.id, github_user, github_project, branch, delete_project=True, - github_refpath=source.refpath, github_kind=source.kind) + github_refpath=refpath, github_kind=kind) return {'task_id': task.task_id, 'project_id': project.id} diff --git a/cloudpebble/ide/static/ide/js/__tests__/github-import-prefill.test.js b/cloudpebble/ide/static/ide/js/__tests__/github-import-prefill.test.js index d7367e7..106c889 100644 --- a/cloudpebble/ide/static/ide/js/__tests__/github-import-prefill.test.js +++ b/cloudpebble/ide/static/ide/js/__tests__/github-import-prefill.test.js @@ -70,6 +70,15 @@ describe('GitHub import deep-link prefill', () => { expect(fields.branch).toBeUndefined(); }); + it('suggests the repo name for /blob/ and /commit/ URLs (tail is a file or SHA, not a name)', () => { + var blob = loadWithPath('/ide/import/github/user/repo/blob/main/src/app.js'); + expect(blob.url).toBe('github.com/user/repo/blob/main/src/app.js'); + expect(blob.name).toBe('repo'); + var commit = loadWithPath('/ide/import/github/user/repo/commit/abc123'); + expect(commit.url).toBe('github.com/user/repo/commit/abc123'); + expect(commit.name).toBe('repo'); + }); + it('keeps the legacy /// deep-link contract', () => { var fields = loadWithPath('/ide/import/github/user/repo/dev'); expect(fields.url).toBe('github.com/user/repo'); diff --git a/cloudpebble/ide/static/ide/js/project_list.js b/cloudpebble/ide/static/ide/js/project_list.js index a930629..3063656 100644 --- a/cloudpebble/ide/static/ide/js/project_list.js +++ b/cloudpebble/ide/static/ide/js/project_list.js @@ -233,8 +233,13 @@ $(function() { // .../import/github///tree//). Hand // the whole thing to the server, which resolves branch vs // subdirectory against the repository's real refs. - $('#import-github-name').val(tail.length > 2 ? tail[tail.length - 1] : parts[4]); - $('#import-github-url').val('github.com/' + parts.slice(3).concat([]).join('/')); + // Suggest the subdirectory name for /tree/ URLs (it may still be + // part of a slashed branch name — the field is editable and the + // server resolves the truth); blob/commit tails are a filename or + // a SHA, so the repository name is the sane suggestion there. + var suggested = (tail[0] == 'tree' && tail.length > 2) ? tail[tail.length - 1] : parts[4]; + $('#import-github-name').val(suggested); + $('#import-github-url').val('github.com/' + parts.slice(3).join('/')); } else { $('#import-github-name').val(parts[3]); $('#import-github-url').val('github.com/' + parts[3] + '/' + parts[4]); diff --git a/cloudpebble/ide/tasks/archive.py b/cloudpebble/ide/tasks/archive.py index 5c6c5c7..1fa59ac 100644 --- a/cloudpebble/ide/tasks/archive.py +++ b/cloudpebble/ide/tasks/archive.py @@ -25,6 +25,11 @@ logger = logging.getLogger(__name__) +# When a root_hint narrows an import to one project, the 400-entry limit +# applies to that subtree — but the whole archive still needs a ceiling to +# bound the root-finding scan over the repository it lives in. +HINTED_ARCHIVE_SCAN_LIMIT = 50000 + def _public_export_url(path): return "/ide/export/%s" % path.lstrip('/') @@ -168,11 +173,18 @@ def do_import_archive(project_id, archive, delete_project=False, wipe_existing=F WORKER_SRC_DIR = 'worker_src/' INCLUDE_SRC_DIR = 'include/' - if len(contents) > 400: + if len(contents) > (400 if root_hint is None else HINTED_ARCHIVE_SCAN_LIMIT): raise InvalidProjectArchiveException("Too many files in zip file.") archive_items = [ArchiveProjectItem(z, x) for x in contents] base_dir, manifest_item = find_project_root_and_manifest(archive_items, root_hint=root_hint) + if root_hint is not None: + # A subdirectory import only takes files under the hinted + # project, so the size limit applies to that subtree — not + # to the whole repository the project happens to live in. + contents = [entry for entry in contents if entry.filename.startswith(base_dir)] + if len(contents) > 400: + raise InvalidProjectArchiveException("Too many files in the project directory.") dir_end = len(base_dir) def make_valid_filename(zip_entry): diff --git a/cloudpebble/ide/tasks/git.py b/cloudpebble/ide/tasks/git.py index 825069b..08168c8 100644 --- a/cloudpebble/ide/tasks/git.py +++ b/cloudpebble/ide/tasks/git.py @@ -5,6 +5,7 @@ from io import BytesIO from urllib.request import urlopen, Request from urllib.error import URLError, HTTPError +from urllib.parse import quote import json import os import logging @@ -65,7 +66,10 @@ def do_import_github(project_id, github_user, github_project, github_branch, del # hardcoded 'master', which broke every main-default repository. github_branch = 'HEAD' - url = "https://github.com/%s/%s/archive/%s.zip" % (github_user, github_project, github_branch) + # quote() the ref: slashes are real URL separators here, but a branch + # name may legally contain '#', '?' or spaces, which would otherwise + # truncate the request path. + url = "https://github.com/%s/%s/archive/%s.zip" % (github_user, github_project, quote(github_branch, safe='/')) auth_url = get_authenticated_archive_url(user, github_user, github_project, github_branch) archive = None @@ -111,6 +115,15 @@ def resolve_ref_and_path(user, github_user, github_project, refpath, kind): if kind == 'commit': return refpath, '' + # A self-qualified refs/heads/... or refs/tags/... spelling pins the + # namespace; strip it HERE so the probe fallback below builds candidates + # without the qualifier (probing 'refs/heads/refs/heads/x' can never hit). + namespaces = ('heads', 'tags') + qualified = refpath.split('/') + if len(qualified) >= 3 and qualified[0] == 'refs' and qualified[1] in namespaces: + namespaces = (qualified[1],) + refpath = '/'.join(qualified[2:]) + ref, path = None, None ref_names = get_ref_names(user, github_user, github_project) if ref_names is not None: @@ -127,8 +140,8 @@ def resolve_ref_and_path(user, github_user, github_project, refpath, kind): for i in range(start, 0, -1): candidate = '/'.join(parts[:i]) if any(file_exists("https://codeload.github.com/%s/%s/zip/refs/%s/%s" - % (github_user, github_project, refkind, candidate)) - for refkind in ('heads', 'tags')): + % (github_user, github_project, refkind, quote(candidate, safe='/'))) + for refkind in namespaces): ref, path = candidate, '/'.join(parts[i:]) break if ref is None: diff --git a/cloudpebble/ide/tests/test_github_import.py b/cloudpebble/ide/tests/test_github_import.py new file mode 100644 index 0000000..07963fa --- /dev/null +++ b/cloudpebble/ide/tests/test_github_import.py @@ -0,0 +1,146 @@ +""" Tests for the GitHub-import ref/path resolution in ide.tasks.git — +resolve_ref_and_path() with the GitHub API mocked out, and the strict +codeload probe fallback with file_exists mocked — plus the /ide/import/github +API flows around linking (add_remote). """ + +import json +from unittest import mock + +from django.test import TestCase + +from ide.models.project import Project +from ide.tasks.git import resolve_ref_and_path +from ide.utils.cloudpebble_test import CloudpebbleTestCase + + +@mock.patch('ide.tasks.git.get_ref_names') +class TestResolveRefAndPath(TestCase): + REFS = ['main', 'develop', 'feature/foo', 'v1.0.0'] + + def test_api_split(self, get_ref_names): + get_ref_names.return_value = self.REFS + self.assertEqual(('main', 'faces/slothvec'), + resolve_ref_and_path(None, 'u', 'r', 'main/faces/slothvec', 'tree')) + + def test_api_split_slashed_branch(self, get_ref_names): + get_ref_names.return_value = self.REFS + self.assertEqual(('feature/foo', 'src'), + resolve_ref_and_path(None, 'u', 'r', 'feature/foo/src', 'tree')) + + def test_blob_resolves_to_the_files_directory(self, get_ref_names): + get_ref_names.return_value = self.REFS + self.assertEqual(('main', 'faces'), + resolve_ref_and_path(None, 'u', 'r', 'main/faces/app.js', 'blob')) + + def test_commit_passthrough(self, get_ref_names): + self.assertEqual(('abc1234', ''), + resolve_ref_and_path(None, 'u', 'r', 'abc1234', 'commit')) + get_ref_names.assert_not_called() + + def test_invalid_path_fails_loud(self, get_ref_names): + get_ref_names.return_value = self.REFS + with self.assertRaises(Exception): + resolve_ref_and_path(None, 'u', 'r', 'main/../etc', 'tree') + + @mock.patch('ide.tasks.git.file_exists') + def test_probe_fallback_longest_first(self, file_exists, get_ref_names): + get_ref_names.return_value = None + probed = [] + + def fake_exists(url): + probed.append(url) + return url.endswith('/zip/refs/heads/feature/foo') + file_exists.side_effect = fake_exists + + self.assertEqual(('feature/foo', 'src'), + resolve_ref_and_path(None, 'u', 'r', 'feature/foo/src', 'tree')) + # Longest candidate first, strict refs-qualified codeload URLs only. + self.assertIn('https://codeload.github.com/u/r/zip/refs/heads/feature/foo/src', probed) + self.assertTrue(all('/zip/refs/' in url for url in probed)) + + @mock.patch('ide.tasks.git.file_exists') + def test_probe_strips_refs_qualifier_and_pins_namespace(self, file_exists, get_ref_names): + get_ref_names.return_value = None + probed = [] + + def fake_exists(url): + probed.append(url) + return url.endswith('/zip/refs/heads/feature/foo') + file_exists.side_effect = fake_exists + + # refs/heads/feature/foo/src: candidates must NOT start with refs/heads/ + # a second time, and refs/tags/ must never be probed. + self.assertEqual(('feature/foo', 'src'), + resolve_ref_and_path(None, 'u', 'r', 'refs/heads/feature/foo/src', 'tree')) + self.assertTrue(all('/zip/refs/heads/' in url for url in probed)) + self.assertTrue(all('refs/heads/refs' not in url for url in probed)) + + @mock.patch('ide.tasks.git.file_exists') + def test_probe_quotes_unsafe_ref_characters(self, file_exists, get_ref_names): + # A branch may legally contain '#' or '?'; raw in a probe URL they + # would truncate the request path into a fragment/query. + get_ref_names.return_value = None + probed = [] + + def fake_exists(url): + probed.append(url) + return url.endswith('/zip/refs/heads/bug%237') + file_exists.side_effect = fake_exists + + self.assertEqual(('bug#7', 'src'), + resolve_ref_and_path(None, 'u', 'r', 'bug#7/src', 'tree')) + self.assertTrue(all('#' not in url for url in probed)) + + @mock.patch('ide.tasks.git.file_exists') + def test_probe_miss_falls_back_to_first_segment(self, file_exists, get_ref_names): + get_ref_names.return_value = None + file_exists.return_value = False + self.assertEqual(('main', 'faces/slothvec'), + resolve_ref_and_path(None, 'u', 'r', 'main/faces/slothvec', 'tree')) + + +@mock.patch('ide.api.project.do_import_github') +class TestImportGithubApi(CloudpebbleTestCase): + """ The linking (add_remote) rules of POST /ide/import/github. """ + + def setUp(self): + self.login() + + def import_repo(self, do_import_github, repo, add_remote='false', branch=''): + do_import_github.delay.return_value.task_id = 'task-id' + return self.client.post('/ide/import/github', { + 'name': 'imported', 'repo': repo, 'branch': branch, 'add_remote': add_remote}) + + def test_branch_only_tree_url_keeps_linking(self, do_import_github): + result = json.loads(self.import_repo( + do_import_github, 'github.com/u/r/tree/main', add_remote='true').content) + self.assertTrue(result['success'], msg=result.get('error')) + project = Project.objects.get(pk=result['project_id']) + self.assertEqual(project.github_repo, 'u/r') + self.assertEqual(project.github_branch, 'main') + args, kwargs = do_import_github.delay.call_args + self.assertEqual(args[3], 'main') + self.assertIsNone(kwargs['github_refpath']) + + def test_subdirectory_with_linking_is_rejected(self, do_import_github): + response = self.import_repo( + do_import_github, 'github.com/u/r/tree/main/faces/x', add_remote='true') + self.assertEqual(response.status_code, 400) + self.assertIn('subdirectory imports', json.loads(response.content)['error']) + do_import_github.delay.assert_not_called() + + def test_commit_with_linking_is_rejected_with_its_own_reason(self, do_import_github): + response = self.import_repo( + do_import_github, 'github.com/u/r/commit/abc123', add_remote='true') + self.assertEqual(response.status_code, 400) + self.assertIn('commit imports', json.loads(response.content)['error']) + do_import_github.delay.assert_not_called() + + def test_subdirectory_without_linking_passes_the_refpath(self, do_import_github): + result = json.loads(self.import_repo( + do_import_github, 'github.com/u/r/tree/main/faces/x').content) + self.assertTrue(result['success'], msg=result.get('error')) + args, kwargs = do_import_github.delay.call_args + self.assertEqual(args[3], '') + self.assertEqual(kwargs['github_refpath'], 'main/faces/x') + self.assertEqual(kwargs['github_kind'], 'tree') diff --git a/cloudpebble/ide/tests/test_github_urls.py b/cloudpebble/ide/tests/test_github_urls.py index ea0cd62..ba68ae7 100644 --- a/cloudpebble/ide/tests/test_github_urls.py +++ b/cloudpebble/ide/tests/test_github_urls.py @@ -94,6 +94,13 @@ def test_legacy_regex_parity(self): 'github.com/user/repo/'): self.assert_parsed(source, 'user', 'repo') + def test_encoded_delimiters_inside_path_survive(self): + # %23/%3F are DATA; only raw '?' and '#' delimit. + self.assert_parsed('github.com/user/repo/tree/main/faces/foo%23bar', + 'user', 'repo', 'tree', 'main/faces/foo#bar') + self.assert_parsed('github.com/user/repo/tree/main/a%3Fb?tab=x#frag', + 'user', 'repo', 'tree', 'main/a?b') + def test_rejects_non_github(self): for source in ('gitlab.com/user/repo', 'https://example.com/user/repo', 'user', ''): self.assertIsNone(parse_github_source(source), source) @@ -148,6 +155,8 @@ def test_collapses(self): self.assertEqual('a/b', normalize_subpath('a/./b')) def test_rejects_escapes(self): - for bad in ('..', '../x', 'a/../../b'): + # 'a/..' cancels out arithmetically, but '..' has no legitimate place + # in a GitHub tree URL — reject rather than normalize to the root. + for bad in ('..', '../x', 'a/../../b', 'a/..', 'a/../b'): with self.assertRaises(ValueError): normalize_subpath(bad) diff --git a/cloudpebble/ide/tests/test_import_archive.py b/cloudpebble/ide/tests/test_import_archive.py index e13e88a..57c600d 100644 --- a/cloudpebble/ide/tests/test_import_archive.py +++ b/cloudpebble/ide/tests/test_import_archive.py @@ -344,3 +344,57 @@ def test_wipe_existing_rolls_back_on_failure(self): project = Project.objects.get(pk=self.project_id) self.assertEqual(project.source_files.count(), 1, "Original files should be preserved after failed import") + + +@mock.patch('ide.models.s3file.s3', fake_s3) +class TestImportArchiveWithRootHint(CloudpebbleTestCase): + """ root_hint imports one project out of a multi-project repository + archive — including repositories bigger than the whole-archive file + limit, where only the hinted subtree's size should matter. """ + + def setUp(self): + self.login() + + @staticmethod + def big_repo_bundle(filler_count): + spec = { + 'repo-main/package.json': make_package(package_options={'name': 'rootproject'}), + 'repo-main/src/main.c': '', + 'repo-main/faces/slothvec/package.json': make_package(package_options={'name': 'sloth'}), + 'repo-main/faces/slothvec/src/main.c': '', + } + for i in range(filler_count): + spec['repo-main/docs/filler-%d.txt' % i] = 'x' + return build_bundle(spec) + + def test_hint_picks_the_nested_project(self): + bundle = self.big_repo_bundle(0) + do_import_archive(self.project_id, bundle, root_hint='faces/slothvec') + project = Project.objects.get(pk=self.project_id) + self.assertEqual(project.app_short_name, 'sloth') + + def test_file_limit_applies_to_the_hinted_subtree_not_the_repo(self): + # 450 filler files: the whole archive is over the 400-entry limit, + # the hinted project is 2 files. + bundle = self.big_repo_bundle(450) + do_import_archive(self.project_id, bundle, root_hint='faces/slothvec') + project = Project.objects.get(pk=self.project_id) + self.assertEqual(project.app_short_name, 'sloth') + + def test_unhinted_big_archive_is_still_rejected(self): + bundle = self.big_repo_bundle(450) + with self.assertRaises(InvalidProjectArchiveException): + do_import_archive(self.project_id, bundle) + + def test_hinted_scan_has_a_ceiling(self): + # The subtree limit must not turn the pre-scan into an unbounded + # walk over arbitrarily huge repositories. + bundle = self.big_repo_bundle(450) + with mock.patch('ide.tasks.archive.HINTED_ARCHIVE_SCAN_LIMIT', 100): + with self.assertRaises(InvalidProjectArchiveException): + do_import_archive(self.project_id, bundle, root_hint='faces/slothvec') + + def test_hint_with_no_project_there_fails(self): + bundle = self.big_repo_bundle(0) + with self.assertRaises(InvalidProjectArchiveException): + do_import_archive(self.project_id, bundle, root_hint='faces/missing') diff --git a/cloudpebble/ide/tests/test_run_github_link.py b/cloudpebble/ide/tests/test_run_github_link.py new file mode 100644 index 0000000..8792de3 --- /dev/null +++ b/cloudpebble/ide/tests/test_run_github_link.py @@ -0,0 +1,54 @@ +""" Tests for the appstore-source → import-deep-link translation in +ide.views.run. parse_github_source is the single authority on accepted +shapes; anything it rejects degrades to a plain repository link when a +user/repo pair is discernible, and to no link at all otherwise. """ + +from django.test import TestCase + +from ide.views.run import _github_import_url + + +class TestGithubImportUrl(TestCase): + def test_plain_repository(self): + self.assertEqual('/ide/import/github/user/repo', + _github_import_url('https://github.com/user/repo')) + + def test_legacy_colon_form(self): + self.assertEqual('/ide/import/github/user/repo', + _github_import_url('github.com:user/repo')) + + def test_tree_deep_link(self): + self.assertEqual('/ide/import/github/user/repo/tree/main/faces/slothvec', + _github_import_url('github.com/user/repo/tree/main/faces/slothvec')) + + def test_blob_deep_link(self): + self.assertEqual('/ide/import/github/user/repo/blob/main/src/app.js', + _github_import_url('https://github.com/user/repo/blob/main/src/app.js')) + + def test_unsafe_path_characters_are_reencoded(self): + # The parser percent-decodes; the deep link must re-encode so the + # href survives as one URL path. + self.assertEqual('/ide/import/github/user/repo/tree/main/my%20dir', + _github_import_url('github.com/user/repo/tree/main/my%20dir')) + self.assertEqual('/ide/import/github/user/repo/tree/main/foo%23bar', + _github_import_url('github.com/user/repo/tree/main/foo%23bar')) + + def test_ssh_port_form_makes_no_wrong_link(self): + # '22/user' must never be read as a user/repo pair. + self.assertEqual('', _github_import_url('ssh://git@github.com:22/user/repo')) + + def test_unrecognized_shapes_fall_back_to_the_repository(self): + for source in ('https://github.com/user/repo/Blob/main/x', + 'https://github.com/user/repo/releases', + 'https://github.com/user/repo.git/issues/5'): + self.assertEqual('/ide/import/github/user/repo', + _github_import_url(source), source) + + def test_url_inside_prose(self): + self.assertEqual( + '/ide/import/github/user/repo/tree/main/faces/x', + _github_import_url('Source: https://github.com/user/repo/tree/main/faces/x, enjoy!')) + + def test_no_github_no_link(self): + self.assertEqual('', _github_import_url('')) + self.assertEqual('', _github_import_url('https://gitlab.com/user/repo')) diff --git a/cloudpebble/ide/utils/github_urls.py b/cloudpebble/ide/utils/github_urls.py index ecde02b..d036f4e 100644 --- a/cloudpebble/ide/utils/github_urls.py +++ b/cloudpebble/ide/utils/github_urls.py @@ -24,7 +24,8 @@ # ref names (split_ref_and_path below). GithubSource = namedtuple('GithubSource', ['user', 'project', 'kind', 'refpath']) -# Accepted forms (query strings and fragments are ignored): +# Accepted forms (raw query strings and fragments are stripped before +# percent-decoding, so encoded delimiters survive inside path segments): # user/repo # github.com/user/repo[.git][/] (bare, http, https, www; the # legacy 'github.com:user/repo' @@ -53,12 +54,11 @@ (?Ptree|blob|commit) (?: / - (?P[^?\#]+?) + (?P.+?) )? )? )? /? - (?:[?\#].*)? $ """, re.VERBOSE) @@ -68,7 +68,11 @@ def parse_github_source(source): :param source: whatever the user gave us (URL, shorthand, deep-link tail). :return: a GithubSource, or None if this isn't a recognizable GitHub source. """ - match = _SOURCE_RE.match(unquote(source.strip())) + # Split the RAW string on the query/fragment delimiters first, THEN + # percent-decode: an encoded '#' or '?' inside a path segment + # (…/tree/main/faces/foo%23bar) is data, not a delimiter. + raw = source.strip().split('#', 1)[0].split('?', 1)[0] + match = _SOURCE_RE.match(unquote(raw)) if match is None: return None kind = match.group('kind') @@ -127,6 +131,11 @@ def normalize_subpath(path): absolute paths (deep links are attacker-suppliable). """ if not path: return '' + if '..' in path.split('/'): + # Reject '..' outright rather than letting cancelling segments + # normalize away — deep links are attacker-suppliable and there is no + # legitimate reason for a GitHub tree URL to contain '..'. + raise ValueError("Invalid project path: %r" % path) normalized = posixpath.normpath(path.strip('/')) if normalized in ('.', ''): return '' diff --git a/cloudpebble/ide/utils/project.py b/cloudpebble/ide/utils/project.py index dcbedf2..49ec06c 100644 --- a/cloudpebble/ide/utils/project.py +++ b/cloudpebble/ide/utils/project.py @@ -129,7 +129,7 @@ def find_project_root_and_manifest(project_items, root_hint=None): # If we didn't find a valid project but we did find a broken manifest file, complain about it specifically. if root_hint: raise InvalidProjectArchiveException( - _("No valid Pebble project found at '%s' in this repository." % root_hint)) + _("No valid Pebble project found at '%s' in this repository.") % root_hint) if invalid_package_path: raise InvalidProjectArchiveException(_("The file %s does not contain a valid JSON object." % invalid_package_path)) else: diff --git a/cloudpebble/ide/views/run.py b/cloudpebble/ide/views/run.py index 8514d27..63a7c59 100644 --- a/cloudpebble/ide/views/run.py +++ b/cloudpebble/ide/views/run.py @@ -1,6 +1,7 @@ import json import logging import re +from urllib.parse import quote as http_quote import requests as http_requests from django.conf import settings @@ -10,6 +11,8 @@ from django.views.decorators.csrf import csrf_exempt, ensure_csrf_cookie from django.views.decorators.http import require_safe +from ide.utils.github_urls import parse_github_source + logger = logging.getLogger(__name__) PLATFORM_DISPLAY_NAMES = { @@ -58,6 +61,31 @@ def _get_pbw_url(app_info): return '%s%s' % (settings.APPSTORE_API_BASE, pbw_file) +def _github_import_url(source): + """Best import deep link for an appstore 'source' field, or ''. + + The field may be prose, so the token containing github.com is picked out + and handed to the import URL parser — the single authority on which + shapes the importer accepts (tree/blob/commit deep links included). + Anything it rejects (ports, /releases/, unrecognized casing) degrades to + a plain repository link when a user/repo pair is discernible, rather + than a deep link that would fail to import.""" + if not source or 'github.com' not in source.lower(): + return '' + token = next((tok.strip('\'"<>(),.;:') for tok in source.split() + if 'github.com' in tok.lower()), '') + parsed = parse_github_source(token) + if parsed: + pieces = [parsed.user, parsed.project] + if parsed.kind: + pieces += [parsed.kind, parsed.refpath] + return '/ide/import/github/%s' % http_quote('/'.join(pieces), safe='/') + gh_match = re.search(r"github\.com/([\w.-]+/[\w.-]+)", token) + if gh_match: + return '/ide/import/github/%s' % gh_match.group(1).removesuffix('.git') + return '' + + @require_safe @ensure_csrf_cookie def run_app(request, app_id): @@ -99,14 +127,7 @@ def run_app(request, app_id): app_hearts = app_info.get('hearts', 0) app_uuid = app_info.get('uuid', '') - # Check if source is a GitHub URL - source = app_info.get('source') or '' - github_import_url = '' - if source and 'github.com/' in source: - # Extract account/project from URL like https://github.com/user/repo - gh_match = re.search(r"github\.com/([\w.-]+/[\w.-]+(?:/(?:tree|blob|commit)/[^\s'\"?#]+)?)", source) - if gh_match: - github_import_url = '/ide/import/github/%s' % gh_match.group(1) + github_import_url = _github_import_url(app_info.get('source') or '') return render(request, 'ide/run.html', { 'app_id': app_id, From 466adba5f663cc586750cca8e1f1a7a6d65e849b Mon Sep 17 00:00:00 2001 From: Emin Deniz Date: Fri, 7 Aug 2026 00:31:04 +0300 Subject: [PATCH 6/9] Address the second review round: namespaces, root hints, earlier narrowing - Resolve slashed /tree/ URLs against the repository's real refs before refusing to link (the refusal stays, conservatively, when the ref list is unavailable) - Pin refs/heads|tags-qualified URLs to their namespace on the API path too: get_ref_names now takes the wanted namespaces, so a cross-namespace tag can no longer longest-prefix-steal a remainder the URL claimed as a branch - Treat the empty resolved path of a refpath-carrying URL as an explicit repository-root hint (e.g. /blob//) instead of falling back to the first-manifest heuristic - Check the root hint before reading manifest contents, so a big repository's out-of-tree manifests are never opened or parsed - Bump the project_list.js cache key so cached clients pick up the deep-link fix Co-Authored-By: Claude Fable 5 --- cloudpebble/ide/api/project.py | 26 +++++++--- cloudpebble/ide/tasks/git.py | 24 +++++++-- cloudpebble/ide/templates/ide/index.html | 2 +- .../ide/tests/test_find_project_root.py | 28 ++++++++++ cloudpebble/ide/tests/test_github_import.py | 52 +++++++++++++++++++ cloudpebble/ide/utils/project.py | 18 +++++-- 6 files changed, 131 insertions(+), 19 deletions(-) diff --git a/cloudpebble/ide/api/project.py b/cloudpebble/ide/api/project.py index 7a6dc7c..c14fe30 100644 --- a/cloudpebble/ide/api/project.py +++ b/cloudpebble/ide/api/project.py @@ -19,7 +19,7 @@ from ide.tasks.archive import create_archive, do_import_archive from ide.tasks.build import run_compile from ide.tasks.gist import import_gist -from ide.tasks.git import do_import_github +from ide.tasks.git import do_import_github, get_ref_names from ide.utils.github_urls import parse_github_source from ide.utils.alloy_templates import list_alloy_templates, build_template_archive from ide.utils.c_templates import list_c_templates, build_c_template_archive @@ -1051,13 +1051,23 @@ def import_github(request): # ignored. branch = '' if add_remote: - # Deliberately NOT suggesting to link later from settings: the - # project model has no notion of a subdirectory, so a linked push - # would re-find a project root in the full repository tree and - # could overwrite a different project there. - if kind == 'commit': - raise BadRequest(_("Linking a repository is not supported for commit imports.")) - raise BadRequest(_("Linking a repository is not supported for subdirectory imports.")) + if kind == 'tree': + # A slashed remainder may still be nothing but a branch name + # ('feature/foo') — resolve against the repository's real + # refs before refusing to link. When the ref list is + # unavailable the refusal below stays, conservatively. + ref_names = get_ref_names(request.user, github_user, github_project) + if ref_names is not None and refpath in ref_names: + branch, refpath, kind = refpath, None, None + if refpath is not None: + # Deliberately NOT suggesting to link later from settings: + # the project model has no notion of a subdirectory, so a + # linked push would re-find a project root in the full + # repository tree and could overwrite a different project + # there. + if kind == 'commit': + raise BadRequest(_("Linking a repository is not supported for commit imports.")) + raise BadRequest(_("Linking a repository is not supported for subdirectory imports.")) try: project = Project.objects.create(owner=request.user, name=name) diff --git a/cloudpebble/ide/tasks/git.py b/cloudpebble/ide/tasks/git.py index 08168c8..77fb2ab 100644 --- a/cloudpebble/ide/tasks/git.py +++ b/cloudpebble/ide/tasks/git.py @@ -87,7 +87,13 @@ def do_import_github(project_id, github_user, github_project, github_branch, del % (github_user, github_project, github_branch) ) - return do_import_archive(project_id, archive.read(), root_hint=github_path or None) + # A URL that carried a refpath selected a directory EXPLICITLY — the + # empty path means "the repository root" (e.g. /blob//), which is a real hint, not the absence of one: without it + # the importer's first-manifest heuristic could pick a nested + # project instead of the root one. + return do_import_archive(project_id, archive.read(), + root_hint=github_path if github_refpath else None) except Exception as e: if delete_project and project is not None: try: @@ -125,7 +131,7 @@ def resolve_ref_and_path(user, github_user, github_project, refpath, kind): refpath = '/'.join(qualified[2:]) ref, path = None, None - ref_names = get_ref_names(user, github_user, github_project) + ref_names = get_ref_names(user, github_user, github_project, namespaces) if ref_names is not None: ref, path = split_ref_and_path(refpath, ref_names) elif '/' in refpath: @@ -157,12 +163,20 @@ def resolve_ref_and_path(user, github_user, github_project, refpath, kind): return ref, path -def get_ref_names(user, github_user, github_project): - """ Branch + tag names, or None when the API is unavailable. """ +def get_ref_names(user, github_user, github_project, namespaces=('heads', 'tags')): + """ Ref names in the requested namespaces, or None when the API is + unavailable. A refs/heads|tags-qualified URL pins the namespace, and the + pin must reach the name list: with both merged, a tag could + longest-prefix-match a remainder the URL explicitly claimed as a branch. """ try: g = get_github(user) if user is not None else Github() repo = g.get_repo("%s/%s" % (github_user, github_project)) - return [b.name for b in repo.get_branches()] + [t.name for t in repo.get_tags()] + names = [] + if 'heads' in namespaces: + names += [b.name for b in repo.get_branches()] + if 'tags' in namespaces: + names += [t.name for t in repo.get_tags()] + return names except Exception: return None diff --git a/cloudpebble/ide/templates/ide/index.html b/cloudpebble/ide/templates/ide/index.html index 6de99ac..54b8b48 100644 --- a/cloudpebble/ide/templates/ide/index.html +++ b/cloudpebble/ide/templates/ide/index.html @@ -202,5 +202,5 @@

{% trans "Import Existing Project" %}

{{ alloy_templates|json_script:"alloy-template-data" }} - + {% endblock %} diff --git a/cloudpebble/ide/tests/test_find_project_root.py b/cloudpebble/ide/tests/test_find_project_root.py index c8f8969..ed790b6 100644 --- a/cloudpebble/ide/tests/test_find_project_root.py +++ b/cloudpebble/ide/tests/test_find_project_root.py @@ -195,6 +195,34 @@ def test_hint_suffix_alone_is_not_enough(self): with self.assertRaises(InvalidProjectArchiveException): self.find(self.REPO, "slothvec") + def test_empty_hint_is_the_explicit_repository_root(self): + # A /blob// URL selects the repository root + # explicitly; the heuristic must not wander into nested projects. + base_dir, manifest = self.find(self.REPO, "") + self.assertEqual(base_dir, "repo-main/") + self.assertEqual(manifest.name, "repo-main/package.json") + + def test_empty_hint_without_root_project_throws(self): + with self.assertRaises(InvalidProjectArchiveException): + self.find(["repo-main/faces/slothvec/package.json", + "repo-main/faces/slothvec/src/"], "") + + def test_hint_never_reads_manifests_outside_it(self): + # The hint must narrow BEFORE manifest contents are read, or a huge + # repository's out-of-tree manifests get opened and parsed for + # nothing. + read_paths = [] + + class RecordingItem(FakeProjectItem): + def read(inner): + read_paths.append(inner.name) + return super(RecordingItem, inner).read() + + base_dir, manifest = find_project_root_and_manifest( + [RecordingItem(item) for item in self.REPO], root_hint="faces/slothvec") + self.assertEqual(base_dir, "repo-main/faces/slothvec/") + self.assertEqual(read_paths, ["repo-main/faces/slothvec/package.json"]) + def test_no_hint_keeps_existing_behavior(self): base_dir, manifest = find_project_root_and_manifest( [FakeProjectItem(item) for item in self.REPO]) diff --git a/cloudpebble/ide/tests/test_github_import.py b/cloudpebble/ide/tests/test_github_import.py index 07963fa..e00ca09 100644 --- a/cloudpebble/ide/tests/test_github_import.py +++ b/cloudpebble/ide/tests/test_github_import.py @@ -75,6 +75,27 @@ def fake_exists(url): self.assertTrue(all('/zip/refs/heads/' in url for url in probed)) self.assertTrue(all('refs/heads/refs' not in url for url in probed)) + def test_qualified_ref_pins_the_namespace_on_the_api_path(self, get_ref_names): + # Branch 'release' and tag 'release/docs' both exist (legal + # cross-namespace names). A refs/heads-qualified URL must only be + # matched against branches, or the tag longest-prefix-wins wrongly. + def by_namespace(user, gh_user, gh_project, namespaces=('heads', 'tags')): + names = [] + if 'heads' in namespaces: + names += ['main', 'release'] + if 'tags' in namespaces: + names += ['release/docs', 'v1.0.0'] + return names + get_ref_names.side_effect = by_namespace + + self.assertEqual(('release', 'docs/app'), + resolve_ref_and_path(None, 'u', 'r', 'refs/heads/release/docs/app', 'tree')) + self.assertEqual(('release/docs', 'app'), + resolve_ref_and_path(None, 'u', 'r', 'refs/tags/release/docs/app', 'tree')) + # Unqualified stays cross-namespace, longest first. + self.assertEqual(('release/docs', 'app'), + resolve_ref_and_path(None, 'u', 'r', 'release/docs/app', 'tree')) + @mock.patch('ide.tasks.git.file_exists') def test_probe_quotes_unsafe_ref_characters(self, file_exists, get_ref_names): # A branch may legally contain '#' or '?'; raw in a probe URL they @@ -136,6 +157,37 @@ def test_commit_with_linking_is_rejected_with_its_own_reason(self, do_import_git self.assertIn('commit imports', json.loads(response.content)['error']) do_import_github.delay.assert_not_called() + @mock.patch('ide.api.project.get_ref_names') + def test_slashed_branch_only_tree_url_keeps_linking(self, get_ref_names, do_import_github): + # /tree/feature/foo where feature/foo is purely a branch: resolving + # against the real refs must keep the linking flow available. + get_ref_names.return_value = ['main', 'feature/foo'] + result = json.loads(self.import_repo( + do_import_github, 'github.com/u/r/tree/feature/foo', add_remote='true').content) + self.assertTrue(result['success'], msg=result.get('error')) + project = Project.objects.get(pk=result['project_id']) + self.assertEqual(project.github_branch, 'feature/foo') + args, kwargs = do_import_github.delay.call_args + self.assertEqual(args[3], 'feature/foo') + self.assertIsNone(kwargs['github_refpath']) + + @mock.patch('ide.api.project.get_ref_names') + def test_slashed_subdirectory_still_rejects_linking(self, get_ref_names, do_import_github): + get_ref_names.return_value = ['main', 'feature/foo'] + response = self.import_repo( + do_import_github, 'github.com/u/r/tree/feature/foo/src', add_remote='true') + self.assertEqual(response.status_code, 400) + self.assertIn('subdirectory imports', json.loads(response.content)['error']) + + @mock.patch('ide.api.project.get_ref_names') + def test_slashed_tree_rejects_linking_when_refs_unavailable(self, get_ref_names, do_import_github): + # No ref list (rate limit, private repo): stay conservative. + get_ref_names.return_value = None + response = self.import_repo( + do_import_github, 'github.com/u/r/tree/feature/foo', add_remote='true') + self.assertEqual(response.status_code, 400) + do_import_github.delay.assert_not_called() + def test_subdirectory_without_linking_passes_the_refpath(self, do_import_github): result = json.loads(self.import_repo( do_import_github, 'github.com/u/r/tree/main/faces/x').content) diff --git a/cloudpebble/ide/utils/project.py b/cloudpebble/ide/utils/project.py index 49ec06c..caee8cb 100644 --- a/cloudpebble/ide/utils/project.py +++ b/cloudpebble/ide/utils/project.py @@ -57,6 +57,11 @@ def _dir_matches_hint(base_dir, root_hint): stripped = base_dir.rstrip('/') if stripped == root_hint: return True + if root_hint == '': + # An explicitly-selected repository root (e.g. a /blob// + # URL naming a root-level file): the only other acceptable location + # is directly inside the archive's single wrapping folder. + return '/' not in stripped suffix = '/' + root_hint return stripped.endswith(suffix) and '/' not in stripped[:-len(suffix)] @@ -87,6 +92,11 @@ def find_project_root_and_manifest(project_items, root_hint=None): continue # Ensure that the file is actually a manifest file if dir_end + len(name) == len(base_dir): + # A root hint pins the project to one directory; check it + # BEFORE reading, so the manifests a big repository keeps + # outside that directory are never opened or parsed. + if root_hint is not None and not _dir_matches_hint(base_dir[:dir_end], root_hint): + continue content = item.read() try: if is_manifest(name, content): @@ -101,11 +111,6 @@ def find_project_root_and_manifest(project_items, root_hint=None): # The base dir is the location of the manifest file without the manifest filename. base_dir = base_dir[:dir_end] - # A root hint pins the project to one directory; manifests anywhere - # else (e.g. the repository root) don't count. - if root_hint and not _dir_matches_hint(base_dir, root_hint): - continue - # If we found a valid package.json, just return. if name == PACKAGE_MANIFEST: return base_dir, manifest_item @@ -130,6 +135,9 @@ def find_project_root_and_manifest(project_items, root_hint=None): if root_hint: raise InvalidProjectArchiveException( _("No valid Pebble project found at '%s' in this repository.") % root_hint) + if root_hint is not None: + raise InvalidProjectArchiveException( + _("No valid Pebble project found at the root of this repository.")) if invalid_package_path: raise InvalidProjectArchiveException(_("The file %s does not contain a valid JSON object." % invalid_package_path)) else: From 916d49d8990e9a277bef6ff48515104ebc73778a Mon Sep 17 00:00:00 2001 From: Emin Deniz Date: Fri, 7 Aug 2026 00:53:03 +0300 Subject: [PATCH 7/9] Apply the deep self-review round: one qualifier authority, kind-aware hints MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Extract split_ref_qualifier() as the single authority for GitHub's refs/heads|tags-qualified spelling (it was implemented twice and skipped in the linking check, which refused qualified branch URLs with a misleading message) - Make the empty resolved path kind-aware: only /blob/ URLs pin the explicit repository root; /tree/ and /commit/ URLs with no subdirectory keep the same heuristic a branch-box import gets, so nested-project repositories import identically through either door - Replace the linking ref-list lookup with a single branch probe (get_ref_names paginates every branch and tag — unbounded inside a web request — and would also have linked tags as branches) - Give blob links their own linking-refusal message - Name the per-project entry limit (PROJECT_ENTRY_LIMIT) instead of duplicating a magic 400, and let a broken-manifest diagnosis win over the generic hint error - Drop the dialog's stale placeholder="master"; decode the suggested project name in deep links - Tests for all of it, including the previously-untested do_import_github glue (HEAD default branch, root_hint per URL kind) and the JS submit handler (no master fallback returns): Python 274, JS 39 Co-Authored-By: Claude Fable 5 --- cloudpebble/ide/api/project.py | 21 +-- .../__tests__/github-import-prefill.test.js | 91 ++++++++++-- cloudpebble/ide/static/ide/js/project_list.js | 4 + cloudpebble/ide/tasks/archive.py | 9 +- cloudpebble/ide/tasks/git.py | 50 +++++-- cloudpebble/ide/templates/ide/index.html | 2 +- cloudpebble/ide/tests/test_github_import.py | 129 ++++++++++++++++-- cloudpebble/ide/tests/test_github_urls.py | 35 +++-- cloudpebble/ide/tests/test_import_archive.py | 12 +- cloudpebble/ide/utils/github_urls.py | 38 ++++-- cloudpebble/ide/utils/project.py | 6 +- 11 files changed, 325 insertions(+), 72 deletions(-) diff --git a/cloudpebble/ide/api/project.py b/cloudpebble/ide/api/project.py index c14fe30..14c509a 100644 --- a/cloudpebble/ide/api/project.py +++ b/cloudpebble/ide/api/project.py @@ -19,8 +19,8 @@ from ide.tasks.archive import create_archive, do_import_archive from ide.tasks.build import run_compile from ide.tasks.gist import import_gist -from ide.tasks.git import do_import_github, get_ref_names -from ide.utils.github_urls import parse_github_source +from ide.tasks.git import do_import_github, branch_exists +from ide.utils.github_urls import parse_github_source, split_ref_qualifier from ide.utils.alloy_templates import list_alloy_templates, build_template_archive from ide.utils.c_templates import list_c_templates, build_c_template_archive from utils.td_helper import send_td_event @@ -1053,12 +1053,15 @@ def import_github(request): if add_remote: if kind == 'tree': # A slashed remainder may still be nothing but a branch name - # ('feature/foo') — resolve against the repository's real - # refs before refusing to link. When the ref list is - # unavailable the refusal below stays, conservatively. - ref_names = get_ref_names(request.user, github_user, github_project) - if ref_names is not None and refpath in ref_names: - branch, refpath, kind = refpath, None, None + # ('feature/foo', or the refs/heads-qualified spelling of + # one) — probe the repository before refusing to link. One + # branch lookup, deliberately not the paginate-every-ref + # list, which is unbounded inside a web request. When the + # probe can't answer, the refusal below stays, conservatively. + namespaces, candidate = split_ref_qualifier(refpath) + if 'heads' in namespaces and branch_exists( + request.user, github_user, github_project, candidate): + branch, refpath, kind = candidate, None, None if refpath is not None: # Deliberately NOT suggesting to link later from settings: # the project model has no notion of a subdirectory, so a @@ -1067,6 +1070,8 @@ def import_github(request): # there. if kind == 'commit': raise BadRequest(_("Linking a repository is not supported for commit imports.")) + if kind == 'blob': + raise BadRequest(_("Linking a repository is not supported for file-link imports.")) raise BadRequest(_("Linking a repository is not supported for subdirectory imports.")) try: diff --git a/cloudpebble/ide/static/ide/js/__tests__/github-import-prefill.test.js b/cloudpebble/ide/static/ide/js/__tests__/github-import-prefill.test.js index 106c889..76d8d62 100644 --- a/cloudpebble/ide/static/ide/js/__tests__/github-import-prefill.test.js +++ b/cloudpebble/ide/static/ide/js/__tests__/github-import-prefill.test.js @@ -6,7 +6,7 @@ import { resolve } from 'path'; // callback immediately, so loading the file executes the deep-link prefill. // Every jQuery method is a chainable no-op except val(), which records per // selector — enough to assert what lands in the import dialog's fields. -function makeJqueryMock() { +function makeJqueryMock(attrs) { var elements = {}; var $ = function(selector) { if (typeof selector === 'function') { @@ -14,7 +14,7 @@ function makeJqueryMock() { return $; } if (!elements[selector]) { - var store = { value: undefined }; + var store = { value: undefined, text: '', clickHandler: null }; var el = new Proxy(store, { get: function(target, prop) { if (prop === 'val') { @@ -24,7 +24,28 @@ function makeJqueryMock() { return el; }; } - if (prop === 'text') return function() { return ''; }; + if (prop === 'text') { + return function(v) { + if (v === undefined) return target.text; + target.text = v; + return el; + }; + } + if (prop === 'click') { + return function(fn) { + if (typeof fn === 'function') target.clickHandler = fn; + else if (target.clickHandler) target.clickHandler(); + return el; + }; + } + if (prop === 'find') return function(sel) { return $(sel); }; + if (prop === 'attr') { + return function(name) { + if ((attrs || {})[selector]) return attrs[selector][name]; + return el; + }; + } + if (prop === 'is') return function() { return false; }; if (prop === 'length') return 0; if (prop === '_isMock') return true; return function() { return el; }; @@ -39,21 +60,39 @@ function makeJqueryMock() { return $; } -function loadWithPath(pathname) { +function load(pathname, attrs) { var code = readFileSync(resolve(__dirname, '..', 'project_list.js'), 'utf8'); - var $ = makeJqueryMock(); + var $ = makeJqueryMock(attrs); + var chain = { then: function() { return chain; }, catch: function() { return chain; } }; + var ajax = { Post: vi.fn(function() { return chain; }), PollTask: vi.fn() }; var fn = new Function( '$', 'jQuery', 'gettext', 'jquery_csrf_setup', 'ga', 'Ajax', 'CloudPebble', 'location', code ); - fn($, $, function(s) { return s; }, vi.fn(), vi.fn(), {}, {}, { pathname: pathname }); + fn($, $, function(s) { return s; }, vi.fn(), vi.fn(), ajax, {}, { pathname: pathname }); + return { $: $, ajax: ajax }; +} + +function loadWithPath(pathname) { + var h = load(pathname); return { - name: $('#import-github-name').val(), - url: $('#import-github-url').val(), - branch: $('#import-github-branch').val() + name: h.$('#import-github-name').val(), + url: h.$('#import-github-url').val(), + branch: h.$('#import-github-branch').val() }; } +// Drives the real import dialog: the active tab claims to be the GitHub +// pane, fields are set, and the Run button's captured handler is fired. +function submitGithubImport(fields) { + var h = load('/ide/', { '#import-prompt .tab-pane.active': { id: 'import-github' } }); + h.$('#import-github-name').val(fields.name); + h.$('#import-github-url').val(fields.url); + h.$('#import-github-branch').val(fields.branch); + h.$('#run-import').click(); + return h; +} + describe('GitHub import deep-link prefill', () => { it('hands a /tree// URL (the appstore Remix shape) to the server whole', () => { var fields = loadWithPath('/ide/import/github/emindeniz99/pebble-signals/tree/main/faces/slothvec'); @@ -79,9 +118,17 @@ describe('GitHub import deep-link prefill', () => { expect(commit.name).toBe('repo'); }); + it('decodes the suggested name but keeps the URL encoded', () => { + var fields = loadWithPath('/ide/import/github/user/repo/tree/main/my%20dir'); + expect(fields.url).toBe('github.com/user/repo/tree/main/my%20dir'); + expect(fields.name).toBe('my dir'); + }); + it('keeps the legacy /// deep-link contract', () => { var fields = loadWithPath('/ide/import/github/user/repo/dev'); expect(fields.url).toBe('github.com/user/repo'); + // parts[3] is the USERNAME — a pre-existing upstream quirk, kept + // bug-compatible on purpose (this PR only preserves the contract). expect(fields.name).toBe('user'); expect(fields.branch).toBe('dev'); }); @@ -104,3 +151,29 @@ describe('GitHub import deep-link prefill', () => { expect(fields.name).toBeUndefined(); }); }); + + +describe('GitHub import submit handler', () => { + it('submits an empty branch untouched (no master fallback)', () => { + var h = submitGithubImport({ name: 'proj', url: 'github.com/user/repo', branch: '' }); + expect(h.ajax.Post).toHaveBeenCalledWith('/ide/import/github', { + name: 'proj', repo: 'github.com/user/repo', branch: '', add_remote: false + }); + }); + + it('lets /tree/ URLs through the prefix check to the server', () => { + var h = submitGithubImport({ + name: 'sloth', url: 'github.com/user/repo/tree/main/faces/slothvec', branch: '' + }); + expect(h.ajax.Post).toHaveBeenCalledWith('/ide/import/github', { + name: 'sloth', repo: 'github.com/user/repo/tree/main/faces/slothvec', + branch: '', add_remote: false + }); + }); + + it('rejects a non-GitHub URL before posting', () => { + var h = submitGithubImport({ name: 'p', url: 'gitlab.com/user/repo', branch: '' }); + expect(h.ajax.Post).not.toHaveBeenCalled(); + expect(h.$('.errors').text()).toContain('GitHub'); + }); +}); diff --git a/cloudpebble/ide/static/ide/js/project_list.js b/cloudpebble/ide/static/ide/js/project_list.js index 3063656..d0f74dd 100644 --- a/cloudpebble/ide/static/ide/js/project_list.js +++ b/cloudpebble/ide/static/ide/js/project_list.js @@ -238,6 +238,10 @@ $(function() { // server resolves the truth); blob/commit tails are a filename or // a SHA, so the repository name is the sane suggestion there. var suggested = (tail[0] == 'tree' && tail.length > 2) ? tail[tail.length - 1] : parts[4]; + // location.pathname keeps percent-encoding; the URL field must + // stay encoded (the server decodes it) but a suggested NAME + // should read as text. Malformed encodings just stay as-is. + try { suggested = decodeURIComponent(suggested); } catch (e) {} $('#import-github-name').val(suggested); $('#import-github-url').val('github.com/' + parts.slice(3).join('/')); } else { diff --git a/cloudpebble/ide/tasks/archive.py b/cloudpebble/ide/tasks/archive.py index 1fa59ac..ae41f01 100644 --- a/cloudpebble/ide/tasks/archive.py +++ b/cloudpebble/ide/tasks/archive.py @@ -25,7 +25,10 @@ logger = logging.getLogger(__name__) -# When a root_hint narrows an import to one project, the 400-entry limit +# Hard limit on how many files an imported project may contain. +PROJECT_ENTRY_LIMIT = 400 + +# When a root_hint narrows an import to one project, PROJECT_ENTRY_LIMIT # applies to that subtree — but the whole archive still needs a ceiling to # bound the root-finding scan over the repository it lives in. HINTED_ARCHIVE_SCAN_LIMIT = 50000 @@ -173,7 +176,7 @@ def do_import_archive(project_id, archive, delete_project=False, wipe_existing=F WORKER_SRC_DIR = 'worker_src/' INCLUDE_SRC_DIR = 'include/' - if len(contents) > (400 if root_hint is None else HINTED_ARCHIVE_SCAN_LIMIT): + if len(contents) > (PROJECT_ENTRY_LIMIT if root_hint is None else HINTED_ARCHIVE_SCAN_LIMIT): raise InvalidProjectArchiveException("Too many files in zip file.") archive_items = [ArchiveProjectItem(z, x) for x in contents] @@ -183,7 +186,7 @@ def do_import_archive(project_id, archive, delete_project=False, wipe_existing=F # project, so the size limit applies to that subtree — not # to the whole repository the project happens to live in. contents = [entry for entry in contents if entry.filename.startswith(base_dir)] - if len(contents) > 400: + if len(contents) > PROJECT_ENTRY_LIMIT: raise InvalidProjectArchiveException("Too many files in the project directory.") dir_end = len(base_dir) diff --git a/cloudpebble/ide/tasks/git.py b/cloudpebble/ide/tasks/git.py index 77fb2ab..375da87 100644 --- a/cloudpebble/ide/tasks/git.py +++ b/cloudpebble/ide/tasks/git.py @@ -24,7 +24,7 @@ from ide.tasks import do_import_archive, run_compile from ide.tasks.archive import get_filename_variant from ide.utils.git import git_sha, git_blob -from ide.utils.github_urls import split_ref_and_path, normalize_subpath +from ide.utils.github_urls import split_ref_qualifier, split_ref_and_path, normalize_subpath from ide.utils.project import find_project_root_and_manifest, BaseProjectItem, InvalidProjectArchiveException, MANIFEST_KINDS from ide.utils.sdk import generate_manifest_dict, generate_manifest, generate_wscript_file, load_manifest_dict, manifest_name_for_project from utils.td_helper import send_td_event @@ -87,13 +87,18 @@ def do_import_github(project_id, github_user, github_project, github_branch, del % (github_user, github_project, github_branch) ) - # A URL that carried a refpath selected a directory EXPLICITLY — the - # empty path means "the repository root" (e.g. /blob//), which is a real hint, not the absence of one: without it - # the importer's first-manifest heuristic could pick a nested - # project instead of the root one. - return do_import_archive(project_id, archive.read(), - root_hint=github_path if github_refpath else None) + # A /blob/ URL names a FILE, so its resolved directory — even the + # empty one, for a root-level file — is an explicit choice, and the + # empty path becomes the explicit repository-root hint. For /tree/ + # and /commit/ URLs an empty path just means "the whole tree": those + # keep the same first-manifest heuristic a branch-box import gets, + # so a repository whose only project is nested imports identically + # through either door. + if github_kind == 'blob': + root_hint = github_path + else: + root_hint = github_path or None + return do_import_archive(project_id, archive.read(), root_hint=root_hint) except Exception as e: if delete_project and project is not None: try: @@ -124,11 +129,7 @@ def resolve_ref_and_path(user, github_user, github_project, refpath, kind): # A self-qualified refs/heads/... or refs/tags/... spelling pins the # namespace; strip it HERE so the probe fallback below builds candidates # without the qualifier (probing 'refs/heads/refs/heads/x' can never hit). - namespaces = ('heads', 'tags') - qualified = refpath.split('/') - if len(qualified) >= 3 and qualified[0] == 'refs' and qualified[1] in namespaces: - namespaces = (qualified[1],) - refpath = '/'.join(qualified[2:]) + namespaces, refpath = split_ref_qualifier(refpath) ref, path = None, None ref_names = get_ref_names(user, github_user, github_project, namespaces) @@ -163,6 +164,29 @@ def resolve_ref_and_path(user, github_user, github_project, refpath, kind): return ref, path +def branch_exists(user, github_user, github_project, branch): + """ True/False whether the branch exists, or None when the API is + unavailable. One request — unlike get_ref_names, which paginates every + branch and tag and so must never run inside a web request. """ + try: + try: + g = get_github(user) if user is not None else Github() + except Exception: + # No linked GitHub account — public repositories still answer + # anonymously. + g = Github() + repo = g.get_repo("%s/%s" % (github_user, github_project)) + try: + repo.get_branch(branch) + return True + except GithubException as e: + if e.status == 404: + return False + raise + except Exception: + return None + + def get_ref_names(user, github_user, github_project, namespaces=('heads', 'tags')): """ Ref names in the requested namespaces, or None when the API is unavailable. A refs/heads|tags-qualified URL pins the namespace, and the diff --git a/cloudpebble/ide/templates/ide/index.html b/cloudpebble/ide/templates/ide/index.html index 54b8b48..f69acf0 100644 --- a/cloudpebble/ide/templates/ide/index.html +++ b/cloudpebble/ide/templates/ide/index.html @@ -173,7 +173,7 @@

{% trans "Import Existing Project" %}

- +
{% if user.github_repo_sync %} diff --git a/cloudpebble/ide/tests/test_github_import.py b/cloudpebble/ide/tests/test_github_import.py index e00ca09..0b87328 100644 --- a/cloudpebble/ide/tests/test_github_import.py +++ b/cloudpebble/ide/tests/test_github_import.py @@ -9,7 +9,7 @@ from django.test import TestCase from ide.models.project import Project -from ide.tasks.git import resolve_ref_and_path +from ide.tasks.git import do_import_github, resolve_ref_and_path from ide.utils.cloudpebble_test import CloudpebbleTestCase @@ -39,7 +39,7 @@ def test_commit_passthrough(self, get_ref_names): def test_invalid_path_fails_loud(self, get_ref_names): get_ref_names.return_value = self.REFS - with self.assertRaises(Exception): + with self.assertRaisesRegex(Exception, 'Invalid project path'): resolve_ref_and_path(None, 'u', 'r', 'main/../etc', 'tree') @mock.patch('ide.tasks.git.file_exists') @@ -157,11 +157,11 @@ def test_commit_with_linking_is_rejected_with_its_own_reason(self, do_import_git self.assertIn('commit imports', json.loads(response.content)['error']) do_import_github.delay.assert_not_called() - @mock.patch('ide.api.project.get_ref_names') - def test_slashed_branch_only_tree_url_keeps_linking(self, get_ref_names, do_import_github): - # /tree/feature/foo where feature/foo is purely a branch: resolving - # against the real refs must keep the linking flow available. - get_ref_names.return_value = ['main', 'feature/foo'] + @mock.patch('ide.api.project.branch_exists') + def test_slashed_branch_only_tree_url_keeps_linking(self, branch_exists, do_import_github): + # /tree/feature/foo where feature/foo is purely a branch: one branch + # probe must keep the linking flow available. + branch_exists.return_value = True result = json.loads(self.import_repo( do_import_github, 'github.com/u/r/tree/feature/foo', add_remote='true').content) self.assertTrue(result['success'], msg=result.get('error')) @@ -171,23 +171,66 @@ def test_slashed_branch_only_tree_url_keeps_linking(self, get_ref_names, do_impo self.assertEqual(args[3], 'feature/foo') self.assertIsNone(kwargs['github_refpath']) - @mock.patch('ide.api.project.get_ref_names') - def test_slashed_subdirectory_still_rejects_linking(self, get_ref_names, do_import_github): - get_ref_names.return_value = ['main', 'feature/foo'] + @mock.patch('ide.api.project.branch_exists') + def test_qualified_branch_tree_url_keeps_linking(self, branch_exists, do_import_github): + # The refs/heads-qualified spelling of the same URL must link too — + # and the probe must see the UNQUALIFIED name. + branch_exists.return_value = True + result = json.loads(self.import_repo( + do_import_github, 'github.com/u/r/tree/refs/heads/feature/foo', add_remote='true').content) + self.assertTrue(result['success'], msg=result.get('error')) + self.assertEqual(branch_exists.call_args[0][3], 'feature/foo') + project = Project.objects.get(pk=result['project_id']) + self.assertEqual(project.github_branch, 'feature/foo') + + @mock.patch('ide.api.project.branch_exists') + def test_tags_qualified_tree_url_rejects_linking(self, branch_exists, do_import_github): + # A tag is not a branch; linking to one makes no sense and the + # probe must not even run. + response = self.import_repo( + do_import_github, 'github.com/u/r/tree/refs/tags/v1.0.0', add_remote='true') + self.assertEqual(response.status_code, 400) + branch_exists.assert_not_called() + + @mock.patch('ide.api.project.branch_exists') + def test_slashed_subdirectory_still_rejects_linking(self, branch_exists, do_import_github): + branch_exists.return_value = False response = self.import_repo( do_import_github, 'github.com/u/r/tree/feature/foo/src', add_remote='true') self.assertEqual(response.status_code, 400) self.assertIn('subdirectory imports', json.loads(response.content)['error']) - @mock.patch('ide.api.project.get_ref_names') - def test_slashed_tree_rejects_linking_when_refs_unavailable(self, get_ref_names, do_import_github): - # No ref list (rate limit, private repo): stay conservative. - get_ref_names.return_value = None + @mock.patch('ide.api.project.branch_exists') + def test_slashed_tree_rejects_linking_when_probe_unavailable(self, branch_exists, do_import_github): + # Probe can't answer (rate limit, private repo): stay conservative. + branch_exists.return_value = None response = self.import_repo( do_import_github, 'github.com/u/r/tree/feature/foo', add_remote='true') self.assertEqual(response.status_code, 400) do_import_github.delay.assert_not_called() + def test_blob_with_linking_gets_its_own_reason(self, do_import_github): + response = self.import_repo( + do_import_github, 'github.com/u/r/blob/main/package.json', add_remote='true') + self.assertEqual(response.status_code, 400) + self.assertIn('file-link imports', json.loads(response.content)['error']) + + def test_url_refpath_wins_over_a_typed_branch(self, do_import_github): + # The URL is authoritative: a conflicting typed branch is dropped. + result = json.loads(self.import_repo( + do_import_github, 'github.com/u/r/tree/main/faces/x', branch='dev').content) + self.assertTrue(result['success'], msg=result.get('error')) + args, kwargs = do_import_github.delay.call_args + self.assertEqual(args[3], '') + self.assertEqual(kwargs['github_refpath'], 'main/faces/x') + + def test_branch_only_tree_url_wins_over_a_typed_branch(self, do_import_github): + result = json.loads(self.import_repo( + do_import_github, 'github.com/u/r/tree/main', branch='dev').content) + self.assertTrue(result['success'], msg=result.get('error')) + args, kwargs = do_import_github.delay.call_args + self.assertEqual(args[3], 'main') + def test_subdirectory_without_linking_passes_the_refpath(self, do_import_github): result = json.loads(self.import_repo( do_import_github, 'github.com/u/r/tree/main/faces/x').content) @@ -196,3 +239,61 @@ def test_subdirectory_without_linking_passes_the_refpath(self, do_import_github) self.assertEqual(args[3], '') self.assertEqual(kwargs['github_refpath'], 'main/faces/x') self.assertEqual(kwargs['github_kind'], 'tree') + + +@mock.patch('ide.tasks.git.do_import_archive') +@mock.patch('ide.tasks.git.urlopen') +@mock.patch('ide.tasks.git.file_exists') +@mock.patch('ide.tasks.git.get_ref_names') +class TestDoImportGithubGlue(CloudpebbleTestCase): + """ The glue in do_import_github itself: which archive URL is fetched + and which root_hint reaches do_import_archive, per URL kind. """ + + def setUp(self): + self.login() + + def run_import(self, get_ref_names, file_exists, urlopen, do_import_archive, + branch='', refpath=None, kind=None, refs=('main',)): + get_ref_names.return_value = list(refs) + file_exists.return_value = True + urlopen.return_value.read.return_value = b'zipbytes' + do_import_github(self.project_id, 'u', 'r', branch, + github_refpath=refpath, github_kind=kind) + url = file_exists.call_args[0][0] + root_hint = do_import_archive.call_args[1]['root_hint'] + return url, root_hint + + def test_empty_branch_imports_the_default_branch(self, *mocks): + # The headline fix: codeload HEAD, not a hardcoded 'master'. + url, root_hint = self.run_import(*mocks) + self.assertTrue(url.endswith('/u/r/archive/HEAD.zip'), url) + self.assertIsNone(root_hint) + + def test_typed_branch_is_fetched_verbatim(self, *mocks): + url, root_hint = self.run_import(*mocks, branch='dev') + self.assertTrue(url.endswith('/u/r/archive/dev.zip'), url) + self.assertIsNone(root_hint) + + def test_tree_with_subdirectory_hints_the_subdirectory(self, *mocks): + url, root_hint = self.run_import(*mocks, refpath='main/faces/x', kind='tree') + self.assertTrue(url.endswith('/u/r/archive/main.zip'), url) + self.assertEqual(root_hint, 'faces/x') + + def test_blob_at_root_hints_the_explicit_root(self, *mocks): + url, root_hint = self.run_import(*mocks, refpath='main/package.json', kind='blob') + self.assertEqual(root_hint, '') + + def test_commit_gets_no_hint(self, *mocks): + # A commit URL selects a revision, not a directory — nested-project + # repos must import exactly like a branch-box import. + url, root_hint = self.run_import(*mocks, refpath='abc123', kind='commit') + self.assertTrue(url.endswith('/u/r/archive/abc123.zip'), url) + self.assertIsNone(root_hint) + + def test_pure_slashed_branch_tree_gets_no_hint(self, *mocks): + # /tree/feature/foo (whole remainder is a branch) must behave like + # typing feature/foo into the branch box. + url, root_hint = self.run_import(*mocks, refpath='feature/foo', kind='tree', + refs=('feature/foo',)) + self.assertTrue(url.endswith('/u/r/archive/feature/foo.zip'), url) + self.assertIsNone(root_hint) diff --git a/cloudpebble/ide/tests/test_github_urls.py b/cloudpebble/ide/tests/test_github_urls.py index ba68ae7..7595ebb 100644 --- a/cloudpebble/ide/tests/test_github_urls.py +++ b/cloudpebble/ide/tests/test_github_urls.py @@ -6,7 +6,7 @@ from unittest import TestCase -from ide.utils.github_urls import parse_github_source, split_ref_and_path, normalize_subpath +from ide.utils.github_urls import parse_github_source, split_ref_qualifier, split_ref_and_path, normalize_subpath class TestParseGithubSource(TestCase): @@ -130,19 +130,36 @@ def test_tag_ref(self): def test_unknown_ref_falls_back_to_first_segment(self): self.assertEqual(('sha123', 'a/b'), split_ref_and_path('sha123/a/b', self.REFS)) - def test_refs_heads_spelling(self): - self.assertEqual(('feature/foo', 'src'), - split_ref_and_path('refs/heads/feature/foo/src', self.REFS)) - - def test_refs_tags_spelling(self): - self.assertEqual(('v1.0.0', 'dir'), - split_ref_and_path('refs/tags/v1.0.0/dir', self.REFS)) - def test_no_ref_list_falls_back_to_first_segment(self): self.assertEqual(('main', 'faces/slothvec'), split_ref_and_path('main/faces/slothvec', None)) +class TestSplitRefQualifier(TestCase): + def test_unqualified_passes_through(self): + self.assertEqual((('heads', 'tags'), 'main/faces'), + split_ref_qualifier('main/faces')) + + def test_refs_heads_pins_and_strips(self): + self.assertEqual((('heads',), 'feature/foo/src'), + split_ref_qualifier('refs/heads/feature/foo/src')) + + def test_refs_tags_pins_and_strips(self): + self.assertEqual((('tags',), 'v1.0.0/dir'), + split_ref_qualifier('refs/tags/v1.0.0/dir')) + + def test_strips_exactly_once(self): + # A branch literally named 'refs/heads/x' stays addressable through + # one extra qualifier — only the outermost is a spelling. + self.assertEqual((('heads',), 'refs/heads/x'), + split_ref_qualifier('refs/heads/refs/heads/x')) + + def test_composes_with_split_ref_and_path(self): + namespaces, refpath = split_ref_qualifier('refs/heads/feature/foo/src') + self.assertEqual(('feature/foo', 'src'), + split_ref_and_path(refpath, ['main', 'feature/foo'])) + + class TestNormalizeSubpath(TestCase): def test_empty_and_root(self): self.assertEqual('', normalize_subpath('')) diff --git a/cloudpebble/ide/tests/test_import_archive.py b/cloudpebble/ide/tests/test_import_archive.py index 57c600d..ef9cc7b 100644 --- a/cloudpebble/ide/tests/test_import_archive.py +++ b/cloudpebble/ide/tests/test_import_archive.py @@ -356,7 +356,7 @@ def setUp(self): self.login() @staticmethod - def big_repo_bundle(filler_count): + def big_repo_bundle(filler_count, filler_dir='docs'): spec = { 'repo-main/package.json': make_package(package_options={'name': 'rootproject'}), 'repo-main/src/main.c': '', @@ -364,7 +364,7 @@ def big_repo_bundle(filler_count): 'repo-main/faces/slothvec/src/main.c': '', } for i in range(filler_count): - spec['repo-main/docs/filler-%d.txt' % i] = 'x' + spec['repo-main/%s/filler-%d.txt' % (filler_dir, i)] = 'x' return build_bundle(spec) def test_hint_picks_the_nested_project(self): @@ -386,6 +386,14 @@ def test_unhinted_big_archive_is_still_rejected(self): with self.assertRaises(InvalidProjectArchiveException): do_import_archive(self.project_id, bundle) + def test_hinted_subtree_over_the_project_limit_is_rejected(self): + # The per-project limit still binds when the hinted project ITSELF + # is huge — the subtree filter must not quietly raise it to the + # scan ceiling. + bundle = self.big_repo_bundle(450, filler_dir='faces/slothvec/docs') + with self.assertRaisesRegex(InvalidProjectArchiveException, 'project directory'): + do_import_archive(self.project_id, bundle, root_hint='faces/slothvec') + def test_hinted_scan_has_a_ceiling(self): # The subtree limit must not turn the pre-scan into an unbounded # walk over arbitrarily huge repositories. diff --git a/cloudpebble/ide/utils/github_urls.py b/cloudpebble/ide/utils/github_urls.py index d036f4e..4c176d5 100644 --- a/cloudpebble/ide/utils/github_urls.py +++ b/cloudpebble/ide/utils/github_urls.py @@ -86,6 +86,23 @@ def parse_github_source(source): return GithubSource(match.group('user'), match.group('project'), kind, refpath) +def split_ref_qualifier(refpath): + """ Strip GitHub's self-qualified "refs/heads/..." / "refs/tags/..." + spelling (which its Raw button emits) off a /tree/ or /blob/ remainder. + + This is the single authority for the rule — the ref resolver and the + import API both need it, and a second copy could silently drift. + + :return: (namespaces, refpath) — the namespaces the URL pinned + (('heads',) or ('tags',), or both when unqualified) and the + remainder without the qualifier. + """ + parts = refpath.split('/') + if len(parts) >= 3 and parts[0] == 'refs' and parts[1] in ('heads', 'tags'): + return (parts[1],), '/'.join(parts[2:]) + return ('heads', 'tags'), refpath + + def split_ref_and_path(refpath, ref_names): """ Split a /tree/ or /blob/ remainder into (ref, path). @@ -99,21 +116,20 @@ def split_ref_and_path(refpath, ref_names): only decides the cross-namespace case (tag "release" vs branch "release/1.0") in favor of the more specific ref. - GitHub's self-qualified spellings ("refs/heads//..." and - "refs/tags//...", which its Raw button emits) are recognized: the - refs/... prefix is stripped and the remainder matched as usual. + Callers strip GitHub's self-qualified refs/heads|tags spelling first, + with split_ref_qualifier() — which also tells them which namespace the + URL pinned, something this function has no use for. - :param refpath: "" or "/" (already stripped of slashes). - :param ref_names: iterable of the repository's branch and tag names. Pass - None when the list is unavailable — the first segment is then taken as - the ref, which is correct for every ref without a slash in its name - (commit SHAs included, since a SHA is never slashed). + :param refpath: "" or "/" (slashes and any refs/... + qualifier already stripped). + :param ref_names: iterable of the repository's ref names in the + namespaces the URL allows. Pass None when the list is unavailable — + the first segment is then taken as the ref, which is correct for + every ref without a slash in its name (commit SHAs included, since a + SHA is never slashed). :return: (ref, path) — path is '' when the remainder is just a ref. """ parts = refpath.split('/') - if len(parts) >= 3 and parts[0] == 'refs' and parts[1] in ('heads', 'tags'): - parts = parts[2:] - refpath = '/'.join(parts) if len(parts) == 1: return refpath, '' if ref_names is not None: diff --git a/cloudpebble/ide/utils/project.py b/cloudpebble/ide/utils/project.py index caee8cb..8601fe2 100644 --- a/cloudpebble/ide/utils/project.py +++ b/cloudpebble/ide/utils/project.py @@ -132,14 +132,16 @@ def find_project_root_and_manifest(project_items, root_hint=None): return base_dir, manifest_item # If we didn't find a valid project but we did find a broken manifest file, complain about it specifically. + # (Under a hint, manifests are only ever read inside the hinted directory, + # so a broken one is always the actionable diagnosis.) + if invalid_package_path: + raise InvalidProjectArchiveException(_("The file %s does not contain a valid JSON object." % invalid_package_path)) if root_hint: raise InvalidProjectArchiveException( _("No valid Pebble project found at '%s' in this repository.") % root_hint) if root_hint is not None: raise InvalidProjectArchiveException( _("No valid Pebble project found at the root of this repository.")) - if invalid_package_path: - raise InvalidProjectArchiveException(_("The file %s does not contain a valid JSON object." % invalid_package_path)) else: raise InvalidProjectArchiveException( _( From efd109118c761c8c7c15edf309531ee1368f59f2 Mon Sep 17 00:00:00 2001 From: Emin Deniz Date: Fri, 7 Aug 2026 05:51:27 +0300 Subject: [PATCH 8/9] Validate slash-free refs before linking; bound the fallback probe loop - /tree/ with "Use as Git remote" now probes the ref first and refuses a definite non-branch (a tag would import fine but leave github_branch pointing at something get_branch() can never find); an unanswerable probe fails open, matching the branch box, which never validated either - Cap the codeload probe fallback at MAX_PROBE_REF_SEGMENTS candidate prefixes so an attacker-supplied deep URL cannot occupy a worker with hundreds of sequential HEAD requests during an API outage Co-Authored-By: Claude Fable 5 --- cloudpebble/ide/api/project.py | 8 ++++- cloudpebble/ide/tasks/git.py | 11 +++++- cloudpebble/ide/tests/test_github_import.py | 38 ++++++++++++++++++++- 3 files changed, 54 insertions(+), 3 deletions(-) diff --git a/cloudpebble/ide/api/project.py b/cloudpebble/ide/api/project.py index 14c509a..70be457 100644 --- a/cloudpebble/ide/api/project.py +++ b/cloudpebble/ide/api/project.py @@ -1042,7 +1042,13 @@ def import_github(request): if refpath and kind == 'tree' and '/' not in refpath: # A slash-free /tree/ is unambiguously a branch or tag — exactly # equivalent to typing it into the branch box, so every existing flow - # (add_remote included) stays available. + # stays available. Linking is the exception: a linked remote does + # pull/push through repo.get_branch(), so a TAG must not be stored as + # github_branch — probe first, and refuse only on a definite "not a + # branch" (an unanswerable probe keeps the branch-box behavior, which + # never validated either). + if add_remote and branch_exists(request.user, github_user, github_project, refpath) is False: + raise BadRequest(_("Linking a repository requires a branch; '%s' is not one.") % refpath) branch, refpath, kind = refpath, None, None elif refpath: # The URL is authoritative for both the ref and the subdirectory (the diff --git a/cloudpebble/ide/tasks/git.py b/cloudpebble/ide/tasks/git.py index 375da87..202447a 100644 --- a/cloudpebble/ide/tasks/git.py +++ b/cloudpebble/ide/tasks/git.py @@ -34,6 +34,10 @@ logger = logging.getLogger(__name__) +# Longest branch name (in slash-separated segments) the codeload probe +# fallback will consider when the ref list is unavailable. +MAX_PROBE_REF_SEGMENTS = 10 + def exception_reason(error): reason = str(error) @@ -144,7 +148,12 @@ def resolve_ref_and_path(user, github_user, github_project, refpath, kind): # (measured: refs/heads/main 200, refs/heads/main/faces 404). parts = refpath.split('/') start = len(parts) if kind == 'tree' else len(parts) - 1 - for i in range(start, 0, -1): + # Bound the loop: no real branch name has anywhere near this many + # slash-separated segments, and without a cap an attacker-supplied + # deep URL would turn the probe into hundreds of sequential HEAD + # requests on the worker. Deep PATHS still resolve fine as long as + # the branch itself fits the cap. + for i in range(min(start, MAX_PROBE_REF_SEGMENTS), 0, -1): candidate = '/'.join(parts[:i]) if any(file_exists("https://codeload.github.com/%s/%s/zip/refs/%s/%s" % (github_user, github_project, refkind, quote(candidate, safe='/'))) diff --git a/cloudpebble/ide/tests/test_github_import.py b/cloudpebble/ide/tests/test_github_import.py index 0b87328..5631245 100644 --- a/cloudpebble/ide/tests/test_github_import.py +++ b/cloudpebble/ide/tests/test_github_import.py @@ -112,6 +112,17 @@ def fake_exists(url): resolve_ref_and_path(None, 'u', 'r', 'bug#7/src', 'tree')) self.assertTrue(all('#' not in url for url in probed)) + @mock.patch('ide.tasks.git.file_exists') + def test_probe_is_bounded_for_deep_refpaths(self, file_exists, get_ref_names): + # An attacker-supplied 60-segment URL must not turn the fallback + # into 60+ sequential probe requests on the worker. + get_ref_names.return_value = None + file_exists.return_value = False + deep = '/'.join('seg%d' % i for i in range(60)) + self.assertEqual(('seg0', '/'.join('seg%d' % i for i in range(1, 60))), + resolve_ref_and_path(None, 'u', 'r', deep, 'tree')) + self.assertLessEqual(file_exists.call_count, 20) + @mock.patch('ide.tasks.git.file_exists') def test_probe_miss_falls_back_to_first_segment(self, file_exists, get_ref_names): get_ref_names.return_value = None @@ -132,7 +143,9 @@ def import_repo(self, do_import_github, repo, add_remote='false', branch=''): return self.client.post('/ide/import/github', { 'name': 'imported', 'repo': repo, 'branch': branch, 'add_remote': add_remote}) - def test_branch_only_tree_url_keeps_linking(self, do_import_github): + @mock.patch('ide.api.project.branch_exists') + def test_branch_only_tree_url_keeps_linking(self, branch_exists, do_import_github): + branch_exists.return_value = True result = json.loads(self.import_repo( do_import_github, 'github.com/u/r/tree/main', add_remote='true').content) self.assertTrue(result['success'], msg=result.get('error')) @@ -171,6 +184,29 @@ def test_slashed_branch_only_tree_url_keeps_linking(self, branch_exists, do_impo self.assertEqual(args[3], 'feature/foo') self.assertIsNone(kwargs['github_refpath']) + @mock.patch('ide.api.project.branch_exists') + def test_slash_free_tag_with_linking_is_rejected(self, branch_exists, do_import_github): + # /tree/v1.0.0 + linking: the import would work (archives accept + # tags), but a linked remote pulls/pushes via get_branch() — a + # definite "not a branch" must refuse linking up front. + branch_exists.return_value = False + response = self.import_repo( + do_import_github, 'github.com/u/r/tree/v1.0.0', add_remote='true') + self.assertEqual(response.status_code, 400) + self.assertIn('requires a branch', json.loads(response.content)['error']) + do_import_github.delay.assert_not_called() + + @mock.patch('ide.api.project.branch_exists') + def test_slash_free_linking_stays_available_when_probe_cannot_answer(self, branch_exists, do_import_github): + # Fail-open on an unanswerable probe: the branch box never validated + # either, and /tree/main must keep linking under a rate limit. + branch_exists.return_value = None + result = json.loads(self.import_repo( + do_import_github, 'github.com/u/r/tree/main', add_remote='true').content) + self.assertTrue(result['success'], msg=result.get('error')) + project = Project.objects.get(pk=result['project_id']) + self.assertEqual(project.github_branch, 'main') + @mock.patch('ide.api.project.branch_exists') def test_qualified_branch_tree_url_keeps_linking(self, branch_exists, do_import_github): # The refs/heads-qualified spelling of the same URL must link too — From b100c5d1f88cc4f1ce24d943ec3804e20d7b2eca Mon Sep 17 00:00:00 2001 From: Emin Deniz Date: Fri, 7 Aug 2026 05:58:41 +0300 Subject: [PATCH 9/9] Keep the linking tests off the network; fix a docstring miscount The slashed subdirectory add_remote test exercised the real branch_exists, which attempts a live GitHub API call on every run (it stayed green either way, but slow and by luck). branch_exists's docstring also claimed one request where it makes two. Co-Authored-By: Claude Fable 5 --- cloudpebble/ide/tasks/git.py | 5 +++-- cloudpebble/ide/tests/test_github_import.py | 4 +++- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/cloudpebble/ide/tasks/git.py b/cloudpebble/ide/tasks/git.py index 202447a..236cb34 100644 --- a/cloudpebble/ide/tasks/git.py +++ b/cloudpebble/ide/tasks/git.py @@ -175,8 +175,9 @@ def resolve_ref_and_path(user, github_user, github_project, refpath, kind): def branch_exists(user, github_user, github_project, branch): """ True/False whether the branch exists, or None when the API is - unavailable. One request — unlike get_ref_names, which paginates every - branch and tag and so must never run inside a web request. """ + unavailable. A constant two API calls — unlike get_ref_names, which + paginates every branch and tag and so must never run inside a web + request. """ try: try: g = get_github(user) if user is not None else Github() diff --git a/cloudpebble/ide/tests/test_github_import.py b/cloudpebble/ide/tests/test_github_import.py index 5631245..c255f58 100644 --- a/cloudpebble/ide/tests/test_github_import.py +++ b/cloudpebble/ide/tests/test_github_import.py @@ -156,7 +156,9 @@ def test_branch_only_tree_url_keeps_linking(self, branch_exists, do_import_githu self.assertEqual(args[3], 'main') self.assertIsNone(kwargs['github_refpath']) - def test_subdirectory_with_linking_is_rejected(self, do_import_github): + @mock.patch('ide.api.project.branch_exists') + def test_subdirectory_with_linking_is_rejected(self, branch_exists, do_import_github): + branch_exists.return_value = False response = self.import_repo( do_import_github, 'github.com/u/r/tree/main/faces/x', add_remote='true') self.assertEqual(response.status_code, 400)