Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds a seafile-download.sh helper to mirror Seafile client downloads by scraping the official download page, downloading files via wget, and pruning stale local artifacts.
Changes:
- Introduces a Bash script that fetches Seafile’s download page and extracts OSS links using an embedded Python parser.
- Downloads files atomically and skips unchanged downloads via a remote size check.
- Removes local files no longer present upstream, bounded by
TUNASYNC_MAX_DELETE.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
939ae1d to
93d3778
Compare
|
Copilot review addressed:
Additionally, the OSS prefix was updated to |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The hostname mismatch prevents downloads, and several cleanup and temporary-file handling issues remain.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (1)
Resolved since last review (5)
WORKDIRallows path traversal if the URL…Content-Lengthis absent (common with redirects, chunked transfer, or some CDN behaviors),…/tmpis vulnerable to clobbering/collision and can be exploited via…
| # The download page currently links to package.seafile.com (it previously | ||
| # used seafile-downloads.oss-cn-shanghai.aliyuncs.com). | ||
| OSS_PREFIX = "package.seafile.com" |
There was a problem hiding this comment.
The page moved: it now links to package.seafile.com (Alibaba ESA CDN/OSS), not seafile-downloads.oss-cn-shanghai.aliyuncs.com. The PR description has been updated to say so, and the code's allowlist already matches package.seafile.com, so this is resolved without a code change.
happyaron
left a comment
happyaron
left a comment
There was a problem hiding this comment.
Checked against the live page: it currently lists 10 client files on package.seafile.com plus the seafile-server tarball (correctly skipped), and a sample file returns 200 OK directly with Content-Length, so the spider/size logic works as intended. The earlier Copilot findings look properly addressed. A few remaining points inline.
Also, the PR description still says the page links to seafile-downloads.oss-cn-shanghai.aliyuncs.com; updating it to package.seafile.com should resolve the open Copilot thread (no code change needed there).
| # files (bounded by TUNASYNC_MAX_DELETE). | ||
| # | ||
| # wget is used (not curl or urllib) because the tunasync Docker bridge | ||
| # network can reach the Seafile AWS origin only via wget. |
There was a problem hiding this comment.
package.seafile.com currently responds with Server: ESA and x-oss-* headers, i.e. Alibaba ESA CDN in front of Aliyun OSS rather than AWS. The curl-vs-wget observation may well still hold, but the comment here (and at line 112, and in the PR description) should probably not name AWS.
There was a problem hiding this comment.
Fixed in c04f802: the comments now say package.seafile.com is Alibaba ESA CDN in front of Aliyun OSS (the curl-vs-wget observation is kept but no longer names AWS). The PR description is updated likewise.
|
|
||
| if (remote_size is not None | ||
| and os.path.exists(target) | ||
| and os.path.getsize(target) == remote_size): |
There was a problem hiding this comment.
Size is the only freshness signal, so a same-name file whose content changes without a size change is never refreshed. That's realistic here: seafile-android-latest.apk is a mutable name. The server already sends ETag, Last-Modified and x-oss-hash-crc64ecma; recording the ETag (or setting mtime from Last-Modified and comparing it) alongside the size would close this.
There was a problem hiding this comment.
Done in c04f802: the spider now also parses the ETag header, and a small .seafile-download.state file in the working dir records {"size", "etag"} per file. A local file is only skipped when both size and recorded ETag still match, so a same-size replacement of seafile-android-latest.apk is now refreshed; files without a recorded ETag are re-downloaded once to establish state.
| # upstream issue cannot wipe the mirror. | ||
| local_files = [f for f in os.listdir(WORKDIR) | ||
| if os.path.isfile(os.path.join(WORKDIR, f))] | ||
| stale = [f for f in local_files if f not in remote_names and not f.endswith(".tmp")] |
There was a problem hiding this comment.
.tmp files are excluded from stale cleanup, but nothing else removes them either. If a run is killed mid-download (timeout, container stop), foo.msi.tmp stays in the mirror indefinitely and is publicly served. Consider removing leftover *.tmp at the start of the run, or including them in the stale set.
There was a problem hiding this comment.
Done in c04f802: leftover *.tmp (and the new unique *.tmp.XXXXXX names) are removed at the start of every run, before any downloads, so a killed run can no longer leave partial files in the served tree.
| oss_urls = [] | ||
| for u in parser.urls: | ||
| full = u if u.startswith("http") else "https://www.seafile.com" + (u if u.startswith("/") else "/" + u) | ||
| if OSS_PREFIX in full and "seafile-server" not in full: |
There was a problem hiding this comment.
Both checks are substring matches over the whole URL, so e.g. a link with package.seafile.com in its query string would pass. Something like urllib.parse.urlparse(full).hostname == "package.seafile.com" and checking seafile-server against the basename would be stricter. Low risk with the current page.
Minor, on line 64: resolving relative links with urllib.parse.urljoin(UPSTREAM, u) would honor TUNASYNC_UPSTREAM_URL instead of hardcoding https://www.seafile.com (no effect today, since all matching links are absolute). oss_urls also isn't deduplicated, so a link appearing twice on the page is spidered twice.
There was a problem hiding this comment.
Done in c04f802: links are resolved with urllib.parse.urljoin(UPSTREAM, u), accepted only when urlparse(full).hostname == 'package.seafile.com' (scheme restricted to http/https), and 'seafile-server' is matched against the URL basename only.
| exit 1 | ||
| } | ||
|
|
||
| python3 - "$WORKDIR" "$MAX_DELETE" "$UPSTREAM" "$PAGE" <<'PY' |
There was a problem hiding this comment.
Nit: other Python-based jobs in this repo are standalone .py scripts (github-release.py, anaconda.py, ...). Shelling out to wget via subprocess works the same from a .py file, and it'd be easier to lint/test than a ~130-line heredoc. The script also ignores WGET_OPTIONS, which other jobs (e.g. virtualbox.sh) pass through to wget. Maintainers' call.
There was a problem hiding this comment.
Happy to follow the maintainers' call here: if you prefer a standalone seafile-download.py (with WGET_OPTIONS passed through to wget, like virtualbox.sh), I'll convert it. Kept as-is for this round to match the existing shell-wrapper pattern, but no objection to the .py rewrite.
|
Re the round-2 finding on the download-host allowlist (
The script's exact-hostname allowlist therefore matches what the page currently serves, and NJU production runs this version successfully. The PR description has been updated to name |
| full = u if u.startswith("http") else "https://www.seafile.com" + (u if u.startswith("/") else "/" + u) | ||
| if OSS_PREFIX in full and "seafile-server" not in full: | ||
| oss_urls.append(full) |
There was a problem hiding this comment.
Fixed in c04f802: the check is now urllib.parse.urlparse(full).hostname == 'package.seafile.com' with the scheme restricted to http/https, so look-alike hosts or query-string matches are rejected; relative and protocol-relative links are normalized with urljoin first.
| tmp = target + ".tmp" | ||
|
|
||
| remote_size, stderr = remote_size_via_spider(url) | ||
| if remote_size is None: | ||
| # Some CDNs strip Content-Length on chunked or 302 responses; we still | ||
| # need to know whether the URL itself is reachable. | ||
| if stderr and "200 OK" not in stderr and "remote file exists" not in stderr.lower(): | ||
| print(f"ERROR: spider {url}: {stderr[-200:]}", file=sys.stderr) | ||
| sys.exit(1) | ||
| # Fall through; we'll download and trust wget to validate. | ||
|
|
||
| if (remote_size is not None | ||
| and os.path.exists(target) | ||
| and os.path.getsize(target) == remote_size): | ||
| continue | ||
|
|
||
| print(f"Downloading: {name} ({remote_size} bytes)" if remote_size is not None | ||
| else f"Downloading: {name}", file=sys.stderr) | ||
| r = subprocess.run( | ||
| ["wget", "-q", "--timeout=30", "--tries=3", "-O", tmp, url], |
There was a problem hiding this comment.
Fixed in c04f802: downloads go to a tempfile.mkstemp file created with O_EXCL in the working dir (unique, non-predictable name), and wget -O writes to that already-existing file, so a pre-planted symlink at a predictable path can no longer redirect the download. Leftover temp files are also removed at the start of each run.
| exit 1 | ||
| } | ||
|
|
||
| python3 - "$WORKDIR" "$MAX_DELETE" "$UPSTREAM" "$PAGE" <<'PY' |
There was a problem hiding this comment.
Fixed in c04f802: the shell now resolves WORKDIR with pwd -P right after cd and passes the absolute path into Python, so a relative TUNASYNC_WORKING_DIR no longer breaks joins/listing.
The official Seafile download page (www.seafile.com/download) embeds direct download links on package.seafile.com (Alibaba ESA CDN in front of Aliyun OSS); there is no rsync, ftp, or directory listing. This script fetches the page with wget, parses the download URLs with Python's html.parser, downloads new/changed files atomically, and removes stale local files bounded by TUNASYNC_MAX_DELETE (default 50). Key design decisions (shaped by review): - Links are resolved with urllib.parse.urljoin against TUNASYNC_UPSTREAM_URL and accepted only when the parsed hostname is exactly package.seafile.com (scheme http/https), so look-alike hosts and query-string matches are rejected. Filenames are URL-decoded and must be plain basenames (no separators, NUL, or dot-dots) before being joined with WORKDIR. seafile-server tarballs are matched against the basename and skipped (out of scope). - Freshness is size plus ETag: a per-file {"size", "etag"} record is kept in .seafile-download.state, and a local file is skipped only when both match, so a same-size content change (e.g. seafile-android-latest.apk) is still refreshed. Files without a recorded ETag (first run with this version) are re-downloaded once to establish state. - Downloads go to a tempfile.mkstemp file (O_EXCL, unpredictable name) inside the working dir and are moved into place with os.replace; wget -O therefore cannot be redirected through a pre-planted symlink. Leftover *.tmp partials from killed runs are removed at the start of every run instead of being served forever. - Stale-file deletion is deferred until after every download has succeeded, so a transient upstream or network failure cannot wipe previously mirrored files. - TUNASYNC_WORKING_DIR is validated and resolved to an absolute path (pwd -P) before being passed into Python. - wget is used deliberately (not curl or urllib): on the tunasync Docker bridge network, curl fails to reach the package.seafile.com edge IPs while wget succeeds. Verified in a container against the live page: 20/20 checks pass (10 client files found, seafile-server skipped, spider/size/ETag logic exercised).
c04f802 to
a36d7b3
Compare
|
Branch cleanup note: this branch has been squashed to a single commit, Review items → how they were addressed (all included in the current diff)Maintainer review (@happyaron):
Copilot rounds:
Some earlier in-thread replies described intermediate states of the branch; please rely on the current diff and this summary. |
Summary
Add
seafile-download.shto mirror Seafile desktop client downloads fromhttps://www.seafile.com/download/.Why
The official Seafile download page links its files on
package.seafile.com(Alibaba ESA CDN in front of Aliyun OSS; it previously usedseafile-downloads.oss-cn-shanghai.aliyuncs.comdirectly). There is no rsync, ftp, or directory listing.How it works
wget(into amktempfile, cleaned up via trap)html.parser; resolve hrefs withurljoinagainstTUNASYNC_UPSTREAM_URLand accept only links whose parsed hostname is exactlypackage.seafile.com(http/https) — look-alike hosts and query-string matches are rejected..rejected)wgetinto atempfile.mkstemp(O_EXCL, unpredictable name) temp file in the working dir +os.replace— a pre-planted symlink cannot redirect the download{"size", "etag"}record is kept in.seafile-download.state; a local file is skipped only when both match, so a same-size content change (e.g.seafile-android-latest.apk) is still refreshed. Files without a recorded ETag (first run with this script version) are re-downloaded once to establish state*.tmppartials from killed runs at the start of every runTUNASYNC_MAX_DELETE(default 50)seafile-serverURLs (matched against the basename; out of scope)Environment variables
TUNASYNC_WORKING_DIRTUNASYNC_UPSTREAM_URLhttps://www.seafile.com/download/TUNASYNC_MAX_DELETE50Note on wget vs curl
wgetis used throughout instead ofcurlbecause, in at least one tunasync Docker bridge network,curlfails to reach thepackage.seafile.comedge IPs whilewgetsucceeds. The choice is deliberate and not just a stylistic preference.Testing
Container run against the live page: 20/20 checks pass — 10 client files discovered,
seafile-servercorrectly skipped, spider/size/ETag logic exercised. NJU production runs this version.Deployment
Runs in the standard
tunathu/tunasync-scriptsimage (needswget+python3, both present). First run with this version re-downloads each file once to establish ETag state.