feat: retrieve RemNote-managed images#27
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the remnote_get_media tool, allowing the retrieval of RemNote-managed local images as MCP-native image content. It adds support for configuring allowed media roots via CLI, environment variables, or TOML, and implements robust path traversal checks, MIME sniffing, and size limits. The review feedback suggests enhancing runtime safety in src/media.ts by adding defensive type checks for the media locator and token inputs, as well as optimizing performance in resolveManagedImage by caching file sizes to avoid redundant stat calls.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| export async function resolveManagedImage( | ||
| locator: MediaLocator, | ||
| roots: string[], | ||
| maxInlineBytes = DEFAULT_MAX_INLINE_BYTES | ||
| ): Promise<ResolvedMedia> { | ||
| if ( | ||
| !Number.isInteger(maxInlineBytes) || | ||
| maxInlineBytes < 1 || | ||
| maxInlineBytes > HARD_MAX_INLINE_BYTES | ||
| ) { | ||
| throw new Error(`maxInlineBytes must be an integer between 1 and ${HARD_MAX_INLINE_BYTES}`); | ||
| } | ||
| validateLocalToken(locator.localToken); | ||
| if (locator.kind !== 'image' || locator.source !== 'remnote_managed_local') { | ||
| throw new Error('Unsupported MIME: only RemNote-managed local images are supported'); | ||
| } | ||
|
|
||
| const matches = new Map<string, string>(); | ||
| for (const configuredRoot of roots) { | ||
| let root: string; | ||
| try { | ||
| root = await realpath(resolve(configuredRoot)); | ||
| } catch (error) { | ||
| const code = (error as NodeJS.ErrnoException).code; | ||
| if (code === 'ENOENT' || code === 'ENOTDIR') continue; | ||
| throw new Error( | ||
| `Media permission/read failure for configured root: ${code ?? String(error)}`, | ||
| { | ||
| cause: error, | ||
| } | ||
| ); | ||
| } | ||
|
|
||
| const candidate = resolve(root, locator.localToken); | ||
| if (!isWithinRoot(root, candidate) || dirname(candidate) !== root) { | ||
| throw new Error('Media path traversal rejected: resolved path escaped the configured root'); | ||
| } | ||
|
|
||
| try { | ||
| const candidateReal = await realpath(candidate); | ||
| if (!isWithinRoot(root, candidateReal) || dirname(candidateReal) !== root) { | ||
| throw new Error('Media path traversal rejected: resolved file escaped the configured root'); | ||
| } | ||
| const fileStat = await stat(candidateReal); | ||
| if (fileStat.isFile()) matches.set(candidateReal, candidateReal); | ||
| } catch (error) { | ||
| if (error instanceof Error && error.message.startsWith('Media path traversal rejected:')) { | ||
| throw error; | ||
| } | ||
| const code = (error as NodeJS.ErrnoException).code; | ||
| if (code === 'ENOENT' || code === 'ENOTDIR') continue; | ||
| throw new Error( | ||
| `Media permission/read failure for ${locator.localToken}: ${code ?? String(error)}`, | ||
| { | ||
| cause: error, | ||
| } | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| if (matches.size === 0) { | ||
| throw new Error(`Media file missing: ${locator.localToken}`); | ||
| } | ||
| if (matches.size > 1) { | ||
| throw new Error(`Media file ambiguous: ${locator.localToken} matched ${matches.size} roots`); | ||
| } | ||
|
|
||
| const filePath = [...matches.values()][0]; | ||
| let fileStat; | ||
| let data: Buffer; | ||
| try { | ||
| fileStat = await stat(filePath); | ||
| if (fileStat.size > maxInlineBytes) { | ||
| throw new Error( | ||
| `Media file oversized: ${fileStat.size} bytes exceeds maxInlineBytes ${maxInlineBytes}` | ||
| ); | ||
| } | ||
| data = await readFile(filePath); | ||
| } catch (error) { | ||
| if (error instanceof Error && error.message.startsWith('Media file oversized:')) throw error; | ||
| const code = (error as NodeJS.ErrnoException).code; | ||
| throw new Error( | ||
| `Media permission/read failure for ${locator.localToken}: ${code ?? String(error)}`, | ||
| { | ||
| cause: error, | ||
| } | ||
| ); | ||
| } | ||
|
|
||
| const mimeType = detectImageMime(data); | ||
| if (!mimeType) { | ||
| throw new Error('Unsupported MIME: allowed image types are png, jpeg, gif, and webp'); | ||
| } | ||
|
|
||
| const { localToken: _localToken, ...metadata } = locator; | ||
| return { | ||
| data: data.toString('base64'), | ||
| mimeType, | ||
| sizeBytes: fileStat.size, | ||
| metadata, | ||
| }; | ||
| } |
There was a problem hiding this comment.
This refactoring addresses two key issues:
- Defensive Programming: Adds a runtime check to ensure
locatoris a valid object before accessing its properties, preventing potential TypeErrors if the bridge returns a malformed payload. - Performance/Efficiency: Avoids a redundant
statcall on the matched file. By storing the file size directly in thematchesmap during the initial check, we eliminate the need to callstata second time before reading the file.
export async function resolveManagedImage(
locator: MediaLocator,
roots: string[],
maxInlineBytes = DEFAULT_MAX_INLINE_BYTES
): Promise<ResolvedMedia> {
if (!locator || typeof locator !== 'object') {
throw new Error('Invalid media locator payload received from bridge');
}
if (
!Number.isInteger(maxInlineBytes) ||
maxInlineBytes < 1 ||
maxInlineBytes > HARD_MAX_INLINE_BYTES
) {
throw new Error('maxInlineBytes must be an integer between 1 and ' + HARD_MAX_INLINE_BYTES);
}
validateLocalToken(locator.localToken);
if (locator.kind !== 'image' || locator.source !== 'remnote_managed_local') {
throw new Error('Unsupported MIME: only RemNote-managed local images are supported');
}
const matches = new Map<string, number>();
for (const configuredRoot of roots) {
let root: string;
try {
root = await realpath(resolve(configuredRoot));
} catch (error) {
const code = (error as NodeJS.ErrnoException).code;
if (code === 'ENOENT' || code === 'ENOTDIR') continue;
throw new Error(
'Media permission/read failure for configured root: ' + (code ?? String(error)),
{
cause: error,
}
);
}
const candidate = resolve(root, locator.localToken);
if (!isWithinRoot(root, candidate) || dirname(candidate) !== root) {
throw new Error('Media path traversal rejected: resolved path escaped the configured root');
}
try {
const candidateReal = await realpath(candidate);
if (!isWithinRoot(root, candidateReal) || dirname(candidateReal) !== root) {
throw new Error('Media path traversal rejected: resolved file escaped the configured root');
}
const fileStat = await stat(candidateReal);
if (fileStat.isFile()) matches.set(candidateReal, fileStat.size);
} catch (error) {
if (error instanceof Error && error.message.startsWith('Media path traversal rejected:')) {
throw error;
}
const code = (error as NodeJS.ErrnoException).code;
if (code === 'ENOENT' || code === 'ENOTDIR') continue;
throw new Error(
'Media permission/read failure for ' + locator.localToken + ': ' + (code ?? String(error)),
{
cause: error,
}
);
}
}
if (matches.size === 0) {
throw new Error('Media file missing: ' + locator.localToken);
}
if (matches.size > 1) {
throw new Error('Media file ambiguous: ' + locator.localToken + ' matched ' + matches.size + ' roots');
}
const [filePath, fileSize] = [...matches.entries()][0];
let data: Buffer;
try {
if (fileSize > maxInlineBytes) {
throw new Error(
'Media file oversized: ' + fileSize + ' bytes exceeds maxInlineBytes ' + maxInlineBytes
);
}
data = await readFile(filePath);
} catch (error) {
if (error instanceof Error && error.message.startsWith('Media file oversized:')) throw error;
const code = (error as NodeJS.ErrnoException).code;
throw new Error(
'Media permission/read failure for ' + locator.localToken + ': ' + (code ?? String(error)),
{
cause: error,
}
);
}
const mimeType = detectImageMime(data);
if (!mimeType) {
throw new Error('Unsupported MIME: allowed image types are png, jpeg, gif, and webp');
}
const { localToken: _localToken, ...metadata } = locator;
return {
data: data.toString('base64'),
mimeType,
sizeBytes: fileSize,
metadata,
};
}| function validateLocalToken(token: string): void { | ||
| if ( | ||
| !token || | ||
| basename(token) !== token || | ||
| token === '.' || | ||
| token === '..' || | ||
| token.normalize('NFC') !== token || | ||
| token.includes('\0') | ||
| ) { | ||
| throw new Error('Media path traversal rejected: local media token must be a basename'); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add a defensive type check to ensure token is a string before performing string operations like normalize or includes. Since the input is cast from unknown at the API boundary, runtime type safety is crucial to prevent unexpected TypeErrors.
| function validateLocalToken(token: string): void { | |
| if ( | |
| !token || | |
| basename(token) !== token || | |
| token === '.' || | |
| token === '..' || | |
| token.normalize('NFC') !== token || | |
| token.includes('\0') | |
| ) { | |
| throw new Error('Media path traversal rejected: local media token must be a basename'); | |
| } | |
| } | |
| function validateLocalToken(token: string): void { | |
| if ( | |
| typeof token !== 'string' || | |
| !token || | |
| basename(token) !== token || | |
| token === '.' || | |
| token === '..' || | |
| token.normalize('NFC') !== token || | |
| token.includes('\\0') | |
| ) { | |
| throw new Error('Media path traversal rejected: local media token must be a basename'); | |
| } | |
| } |
Persistent exact-title fixtures derive Rem, property, field, and media IDs without per-machine test configuration. Refs #27
|
Thanks for the contribution. I pulled the changes from this PR and the companion bridge PR, remnote-mcp-bridge#44, into these local feature branches for validation:
On top of the submitted changes, I addressed the review findings: hardened locator validation and file access, bound locators to the requested Rem, added CLI parity for managed-media retrieval, improved error handling, and expanded unit/integration coverage. I also replaced the media/table test environment variables and Rem-ID configuration with convention-based fixtures:
The fixture setup and test behavior are documented in: Missing fixtures now skip only their dependent workflows, and direct MCP, MCPB, and CLI suites continue running so all missing or malformed fixtures are reported together. From my side, both PRs would otherwise be ready to merge. However, I have not been able to make the managed-media integration test run successfully. The fixture is found by name, but the bridge-serialized Could you please clarify:
This distinction is currently unclear from the PR, and it blocks final end-to-end validation of the media path. |
Run all transports and skip only fixture-dependent workflows so one missing fixture does not hide unrelated coverage. Refs #27
|
=> I suggest you to checkout my mentioned feature branches, thy to make integration tests work and explain what is missing to make it work on my side... |
2 participants
Retrieve RemNote-managed images
What
Adds
remnote_get_mediato fetch a specific image attachment on demand, returning MCP-native image content. Newsrc/media.ts(retrieval + size cap + stale-ID handling),config.ts(default 20 MB limit, configurable up to 100 MB), plus type and transport wiring.Failure behavior
Scope
Testing
Sync
Ships with bridge PR #44 (capability
media.images.v1).Original generated description
Summary
remnote_get_mediawith MCP-native image contentremnote_read_notemedia.images.v1capabilitySafety
Validation
npm run typechecknpm test(566 tests)npm run lintnpm run format:checknpm run buildCompanion PR
Requires the synchronized
remnote-mcp-bridgemedia metadata PR.