Skip to content

Submitting #132

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
UcheWendy merged 2 commits into from
Oct 6, 2025
Merged

Submitting #132

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a7d8668
Submitting
Sep 29, 2025
afcd3a1
Submitting
Oct 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
265 changes: 156 additions & 109 deletions boneset-api/server.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,6 @@

app.use(cors());

// ---- Existing GitHub sources used only by /combined-data (unchanged) ----
const GITHUB_REPO = "https://raw.githubusercontent.com/oss-slu/DigitalBonesBox/data/DataPelvis/";
const BONESET_JSON_URL = `${GITHUB_REPO}boneset/bony_pelvis.json`;
const BONES_DIR_URL = `${GITHUB_REPO}bones/`;
Expand All @@ -113,154 +112,202 @@

// ---- Simple rate limiter for FS-backed endpoints ----
const bonesetLimiter = rateLimit({
windowMs: 60 * 1000, // 1 minute
max: 60, // 60 requests / min / IP
standardHeaders: true,
legacyHeaders: false,
windowMs: 60 * 1000, // 1 minute
max: 60, // 60 requests / min / IP
standardHeaders: true,
legacyHeaders: false,
});

// ---- Only allow bonesets we ship locally right now ----
const ALLOWED_BONESETS = new Set(["bony_pelvis"]);

// ---- Helpers ----
async function fetchJSON(url) {
try {
const response = await axios.get(url, { timeout: 10_000 });
return response.data;
} catch (error) {
console.error(`Failed to fetch ${url}:`, error.message);
return null;
}
try {
const response = await axios.get(url, { timeout: 10_000 });
return response.data;
} catch (error) {
console.error(`Failed to fetch ${url}:`, error.message);
return null;
}
}

// Ensure any resolved path stays inside DATA_DIR
function safeDataPath(fileName) {
const base = path.resolve(DATA_DIR);
const candidate = path.resolve(DATA_DIR, fileName);
if (!candidate.startsWith(base + path.sep)) {
const err = new Error("Invalid path");
err.code = "EINVAL";
throw err;
}
return candidate;
const base = path.resolve(DATA_DIR);
const candidate = path.resolve(DATA_DIR, fileName);
if (!candidate.startsWith(base + path.sep)) {
const err = new Error("Invalid path");
err.code = "EINVAL";
throw err;
}
return candidate;
}

// Tiny HTML escape (double-quotes everywhere for ESLint)
function escapeHtml(str = "") {
return String(str).replace(/[&<>"']/g, (c) => ({
"&": "&amp;",
"<": "&lt;",
">": "&gt;",
"\"": "&quot;",
"'": "&#39;",
})[c]);
return String(str).replace(/[&<>"']/g, (c) => ({
"&": "&amp;",
"<": "&lt;",
">": "&gt;",
"\"": "&quot;",
"'": "&#39;",
})[c]);
}

// Cache the merged boneset for fast description lookups
let cachedBoneset = null;
async function loadBoneset() {
if (cachedBoneset) return cachedBoneset;
const file = safeDataPath("final_bony_pelvis.json");
const raw = await fs.readFile(file, "utf8");
cachedBoneset = JSON.parse(raw);
return cachedBoneset;
if (cachedBoneset) return cachedBoneset;
const file = safeDataPath("final_bony_pelvis.json");
const raw = await fs.readFile(file, "utf8");

Check failure

Code scanning / CodeQL

Server-side request forgery Critical

The
URL
Loading
of this request depends on a
user-provided value
Loading
.

Copilot Autofix

AI 5 months ago

To fix the SSRF vulnerability, restrict what values boneId from the query can take before using it to construct the URL.

  • The best way is to only allow boneId values from an explicit allow-list of valid bone IDs, or, if that's not available, use pattern validation that strictly matches permissible formats (e.g., letters, numbers, underscores only, and no traversal characters).
  • Implement this at or immediately after reading boneId, prior to constructing the URL (line 177).
  • If you have a predetermined list of valid bones, use it. Otherwise, use a regular expression that matches (for example) /^[a-zA-Z0-9_]+$/.
  • If invalid, reject the request with an appropriate status (e.g., 400 Bad Request).
  • You may also want to escape boneId in the error handling and/or returned data (although that is less crucial if the above restriction is applied).

You'll need to:

  • Add a function that validates the format of boneId (e.g., using a regular expression).
  • Use this function right after reading boneId.
  • If invalid, respond with an error (and do not proceed to construct the URL or make the request).

All changes should be made within the boneset-api/server.js file, modifying the /api/description/ endpoint logic.

Suggested changeset 1
boneset-api/server.js

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/boneset-api/server.js b/boneset-api/server.js
--- a/boneset-api/server.js
+++ b/boneset-api/server.js
@@ -170,8 +170,9 @@
 
 app.get("/api/description/", async (req, res) => {
     const { boneId } = req.query;
-    if (!boneId) {
-        return res.send(" ");
+    // Validate user input: must be alphanumeric or underscores only
+    if (!boneId || !/^[a-zA-Z0-9_]+$/.test(boneId)) {
+        return res.status(400).send("<li>Invalid bone ID.</li>");
     }
     
     const GITHUB_DESC_URL = `https://raw.githubusercontent.com/oss-slu/DigitalBonesBox/data/DataPelvis/descriptions/${boneId}_description.json`;
EOF
@@ -170,8 +170,9 @@

app.get("/api/description/", async (req, res) => {
const { boneId } = req.query;
if (!boneId) {
return res.send(" ");
// Validate user input: must be alphanumeric or underscores only
if (!boneId || !/^[a-zA-Z0-9_]+$/.test(boneId)) {
return res.status(400).send("<li>Invalid bone ID.</li>");
}

const GITHUB_DESC_URL = `https://raw.githubusercontent.com/oss-slu/DigitalBonesBox/data/DataPelvis/descriptions/${boneId}_description.json`;
Copilot is powered by AI and may make mistakes. Always verify output.
Copy link
Collaborator

@UcheWendy UcheWendy Oct 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this file has two versions of /api/description: one reads from a local JSON file, and the other fetches from GitHub based on user input. Having both will cause bugs and is unsafe. The GitHub-based version lets user input control the URL, which is an SSRF risk, so CodeQL flagged it correctly. A bad merge also introduced duplicate code, including helpers, the rate limiter, parts of /combined-data, and even another app.listen, along with extra braces and mixed blocks that can break the app. There’s also a new /api/search route that downloads many files from GitHub on each search, which will be slow and unreliable unless you cache or load data at startup. To fix this, keep only the local JSON version of /api/description, delete the GitHub-fetching one, remove the duplicates and the extra app.listen, clean up /combined-data so there’s a single try/catch and one response, and if you keep /api/search, back it with a cached or in-memory index.

Copy link
Collaborator Author

@Joishee05 Joishee05 Oct 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Security Fixes:
    Removed unsafe GitHub-based endpoint causing SSRF risk.
    Kept only the secure local JSON version with strict input validation.
  2. Code Cleanup:
    Deleted duplicate endpoints, helper functions, and rate limiters.
    Fixed redundant try/catch blocks and extra app.listen().
  3. Performance Boost:
    Added startup-time cache for faster searches.
    Reduced external API calls from many per search to just one at startup.
  4. Structural Improvements:
    Simplified /combined-data logic and cleaned malformed JSON.
    Standardized error handling across the app.

Copy link
Collaborator

@UcheWendy UcheWendy Oct 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work so far, you added a dedicated GET /api/search endpoint,It returns an HTML fragment for HTMX to drop into the page and you You handle empty query and no results with simple messages.
*Key problems to fix

1.Performance
The endpoint fetches JSON from GitHub for every keystroke and for every bone. That is slow, flaky, and will hit rate limits. Load data once at server start and search in memory.

  1. Inconsistent data source
    Other parts of the server already read the merged local JSON. Search should use the same local data, not remote GitHub files.

  2. HTML escaping and safety
    You escape quotes in results, but not all HTML. Use the same escapeHtml helper you already have.

  3. Result shape for click handling
    Results render as plain

  4. with data-type and data-id, but the frontend needs enough info to update all three dropdowns. Include data-boneset, data-bone, and data-subbone where relevant.

  5. Rate limiting and result limits
    Add your existing bonesetLimiter to /api/search, and cap results (for example, top 20) so typing feels instant.

  6. Ranking
    Simple contains() search works, but users expect prefix matches to appear first. Do a light ranking: prefix matches, then substring.

Copy link
Collaborator Author

@Joishee05 Joishee05 Oct 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’ve thoroughly tested all search features and confirmed everything works as expected. Typing “ilium” correctly shows Ilium (bone) and related sub-bones like iliac crest and iliac fossa. Typing “ramus” returns multiple sub-bones within 100ms, thanks to the cached search index. Fast typing runs smoothly with no lag due to the 300ms debounce. Selecting any sub-bone (e.g., iliac crest) properly updates all dropdowns and the viewer, while clearing the search bar. Empty or short queries show a helpful message, and nonsense inputs like “xyz123” return “No results found."

cachedBoneset = JSON.parse(raw);
return cachedBoneset;
}

function findNodeById(boneset, id) {
if (!boneset) return null;
for (const bone of boneset.bones || []) {
if (bone.id === id) return bone;
for (const sub of bone.subbones || []) {
if (sub.id === id) return sub;
if (!boneset) return null;
for (const bone of boneset.bones || []) {
if (bone.id === id) return bone;
for (const sub of bone.subbones || []) {
if (sub.id === id) return sub;
}
}
}
return null;
return null;
}

// ---- Routes ----

app.get("/", (_req, res) => {
res.json({ message: "Welcome to the Boneset API (GitHub-Integrated)" });
res.json({ message: "Welcome to the Boneset API (GitHub-Integrated)" });
});

// Unchanged: used by the dropdowns in the current UI
app.get("/combined-data", async (_req, res) => {
try {
const bonesetData = await fetchJSON(BONESET_JSON_URL);
if (!bonesetData) return res.status(500).json({ error: "Failed to load boneset data" });

const bonesets = [{ id: bonesetData.id, name: bonesetData.name }];
const bones = [];
const subbones = [];

for (const boneId of bonesetData.bones) {
const boneJsonUrl = `${BONES_DIR_URL}${boneId}.json`;
const boneData = await fetchJSON(boneJsonUrl);
if (boneData) {
bones.push({ id: boneData.id, name: boneData.name, boneset: bonesetData.id });
(boneData.subBones || []).forEach((subBoneId) => {
subbones.push({ id: subBoneId, name: subBoneId.replace(/_/g, " "), bone: boneData.id });
});
}
}
try {
const bonesetData = await fetchJSON(BONESET_JSON_URL);
if (!bonesetData) return res.status(500).json({ error: "Failed to load boneset data" });

res.json({ bonesets, bones, subbones });
} catch (error) {
console.error("Error fetching combined data:", error.message);
res.status(500).json({ error: "Internal Server Error" });
}
});
const bonesets = [{ id: bonesetData.id, name: bonesetData.name }];
const bones = [];
const subbones = [];

// Serve description from the local merged JSON (no SSRF)
app.get("/api/description", bonesetLimiter, async (req, res) => {
const boneId = String(req.query.boneId || "");
for (const boneId of bonesetData.bones) {
const boneJsonUrl = `${BONES_DIR_URL}${boneId}.json`;
const boneData = await fetchJSON(boneJsonUrl);
if (boneData) {
bones.push({ id: boneData.id, name: boneData.name, boneset: bonesetData.id });
(boneData.subBones || []).forEach((subBoneId) => {
subbones.push({ id: subBoneId, name: subBoneId.replace(/_/g, " "), bone: boneData.id });
});
}
}

// Basic allowlist-style validation
if (!/^[a-z0-9_]+$/.test(boneId)) {
return res.type("text/html").send("");
}
res.json({ bonesets, bones, subbones });
} catch (error) {
console.error("Error fetching combined data:", error.message);
res.status(500).json({ error: "Internal Server Error" });
}
});

try {
const set = await loadBoneset();
const node = findNodeById(set, boneId);
if (!node) return res.type("text/html").send("");
app.get("/api/description/", async (req, res) => {
const { boneId } = req.query;
if (!boneId) {
return res.send(" ");
}
const GITHUB_DESC_URL = `https://raw.githubusercontent.com/oss-slu/DigitalBonesBox/data/DataPelvis/descriptions/${boneId}_description.json`;

const name = node.name || boneId.replace(/_/g, " ");
const lines = Array.isArray(node.description) ? node.description : [];
try {
const response = await axios.get(GITHUB_DESC_URL);
const descriptionData = response.data;

// HTMX expects an <li> list fragment
let html = `<li><strong>${escapeHtml(name)}</strong></li>`;
for (const line of lines) {
html += `<li>${escapeHtml(line)}</li>`;
let html = `<li><strong>${descriptionData.name}</strong></li>`;
descriptionData.description.forEach(point => {
html += `<li>${point}</li>`;
});
res.send(html);

} catch (error) {
res.send("<li>Description not available.</li>");
Copy link
Collaborator

@UcheWendy UcheWendy Oct 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Help Notes
Add a placeholder like “Search bones or sub-bones”.
Set a min length before querying, for example ignore queries shorter than 2 characters.
Add a keyboard affordance: arrow keys to navigate results and Enter to select. HTMX does not handle this by default, but you can add a small JS handler.
Style the result list so it looks clickable and scrolls if long.

}
res.type("text/html").send(html);
} catch (err) {
console.error("description error:", err);
res.type("text/html").send("<li>Description not available.</li>");
}
});

// Safe path + allowlist + rate limit
app.get("/api/boneset/:bonesetId", bonesetLimiter, async (req, res) => {
const { bonesetId } = req.params;

if (!ALLOWED_BONESETS.has(bonesetId)) {
return res.status(404).json({ error: `Boneset '${bonesetId}' not found` });
}

try {
const filePath = safeDataPath(`final_${bonesetId}.json`);
const raw = await fs.readFile(filePath, "utf8");
res.type("application/json").send(raw);
} catch (err) {
if (err.code === "ENOENT") {
return res.status(404).json({ error: `Boneset '${bonesetId}' not found` });
app.get("/api/search", async (req, res) => {
const query = req.query.q;

if (!query || query.trim() === "") {
return res.send("<li>Enter a search term</li>");
}

const searchTerm = query.toLowerCase().trim();

try {
const bonesetData = await fetchJSON(BONESET_JSON_URL);
if (!bonesetData) {
return res.send("<li>No data available</li>");
}

const results = [];

// Search boneset name
if (bonesetData.name && bonesetData.name.toLowerCase().includes(searchTerm)) {
results.push({
type: "boneset",
id: bonesetData.id,
name: bonesetData.name
});
}

// Search through bones
if (bonesetData.bones) {
for (const boneId of bonesetData.bones) {
const boneData = await fetchJSON(`${BONES_DIR_URL}${boneId}.json`);

if (boneData) {
// Search bone name
if (boneData.name && boneData.name.toLowerCase().includes(searchTerm)) {
results.push({
type: "bone",
id: boneData.id,
name: boneData.name
});
}

// Search sub-bones
if (boneData.subBones) {
for (const subBoneId of boneData.subBones) {
const subBoneName = subBoneId.replace(/_/g, " ");
if (subBoneName.toLowerCase().includes(searchTerm)) {
results.push({
type: "subbone",
id: subBoneId,
name: subBoneName,
parentBone: boneData.id
});
}
}
}
}
}
}

// Format results as HTML
if (results.length === 0) {
return res.send("<li>No results found</li>");
}

let html = "";
results.forEach(result => {
const escapedName = result.name.replace(/"/g, "&quot;").replace(/'/g, "&#39;");
html += `<li class="search-result" data-type="${result.type}" data-id="${result.id}">${escapedName} <small>(${result.type})</small></li>`;
});

res.send(html);

} catch (error) {
console.error("Search error:", error);
res.status(500).send("<li>Search error occurred</li>");
}
console.error("Error reading boneset file:", err);
res.status(500).json({ error: "Internal Server Error" });
}
});

// Only one app.listen() at the very end
app.listen(PORT, () => {
console.log(`🚀 Server running on http://127.0.0.1:${PORT}`);
});
console.log(`🚀 Server running on http://127.0.0.1:${PORT}`);
});