Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
20 changes: 20 additions & 0 deletions npm_modules/cli/src/debugger/server.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -524,6 +524,26 @@ describe('debugger server', () => {
expect(responseBody.logs.map(log => log.message)).toEqual(['exact application']);
});

it('does not expose absolute log paths in filesystem error responses', async () => {
const logsDirectory = path.join(assetRoot, 'missing', 'logs');
const consoleWarn = spyOn(console, 'warn');
debuggerServer = await startDebuggerServer({
assetRoot,
host: '127.0.0.1',
logsDirectory,
port: await getFreePort(),
strictPort: true,
});

const result = await request(new URL('/api/runtime-logs', debuggerServer.url).toString(), GET_REQUEST_OPTIONS);
const responseBody = JSON.parse(result.body) as { error: string };

expect(result.statusCode).toBe(500);
expect(responseBody.error).toBe('A local filesystem operation failed. See the debugger server output for details.');
expect(result.body).not.toContain(logsDirectory);
expect(consoleWarn).toHaveBeenCalledWith(jasmine.stringContaining(logsDirectory));
});

it('reads the logs directory through the shared YAML config parser', async () => {
const testHome = path.join(assetRoot, 'home');
const logsDirectory = path.join(testHome, 'logs#debug');
Expand Down
24 changes: 20 additions & 4 deletions npm_modules/cli/src/debugger/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,22 @@ function errorPayload(error: unknown): { error: string } {
};
}

function isFileSystemError(error: unknown): error is NodeJS.ErrnoException {
if (!(error instanceof Error)) return false;
const fileSystemError = error as NodeJS.ErrnoException;
return (
typeof fileSystemError.code === 'string' &&
typeof fileSystemError.path === 'string' &&
typeof fileSystemError.syscall === 'string'
);
}

function clientErrorPayload(error: unknown): { error: string } {
if (!isFileSystemError(error)) return errorPayload(error);
console.warn(`Debugger filesystem error: ${errorPayload(error).error}`);
return { error: 'A local filesystem operation failed. See the debugger server output for details.' };
}

function isValidSnapshotBase64(value: string): boolean {
if (!value || value.length % 4 === 1) return false;
const firstPaddingIndex = value.indexOf('=');
Expand Down Expand Up @@ -841,7 +857,7 @@ async function streamRuntimeLogs(

if (nextLogs.length > 0) sendSse(response, 'logs', { logs: nextLogs });
} catch (error) {
sendSse(response, 'stream-error', errorPayload(error));
sendSse(response, 'stream-error', clientErrorPayload(error));
}
}

Expand Down Expand Up @@ -899,7 +915,7 @@ async function collectClientContexts(
try {
contexts = await conn.listContexts(client.client_id);
} catch (error) {
contextError = errorPayload(error).error;
contextError = clientErrorPayload(error).error;
}
}

Expand Down Expand Up @@ -938,7 +954,7 @@ async function inspectPort(port: number): Promise<{
portName: portName(port),
connected: false,
clients: [],
error: errorPayload(error).error,
error: clientErrorPayload(error).error,
};
}
}
Expand Down Expand Up @@ -1375,7 +1391,7 @@ async function handleApi(request: IncomingMessage, response: ServerResponse, url

sendJson(response, 404, { error: `Unknown API route ${url.pathname}` });
} catch (error) {
sendJson(response, error instanceof ApiRequestError ? error.statusCode : 500, errorPayload(error));
sendJson(response, error instanceof ApiRequestError ? error.statusCode : 500, clientErrorPayload(error));
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,39 @@ describe('IRenderedVirtualNodeData', () => {
expect(data.component?.viewModel).toContain('Set(');
});

it('replaces values at the maximum component debug depth with an omission marker', () => {
const data = createDetailedData(
createComponentNode(
{
level1: { level2: { level3: { level4: 'too-deep' } } },
sibling: 'visible',
},
{},
),
);

expect(data.component?.viewModel).toContain('level4: ...');
expect(data.component?.viewModel).not.toContain('too-deep');
expect(data.component?.viewModel).toContain('sibling: "visible"');
});

it('limits arrays and objects to 50 serialized items with omission markers', () => {
const items = Array.from({ length: 52 }, (_value, index) => `item-${index}`);
const properties: Record<string, number> = {};
for (let index = 0; index < 52; index++) {
properties[`property${index}`] = index;
}

const data = createDetailedData(createComponentNode({ items, properties }, {}));

expect(data.component?.viewModel).toContain('"item-49"');
expect(data.component?.viewModel).not.toContain('"item-50"');
expect(data.component?.viewModel).toContain('... 2 more item(s) ...');
expect(data.component?.viewModel).toContain('property49: 49');
expect(data.component?.viewModel).not.toContain('property50: 50');
expect(data.component?.viewModel).toContain('... more properties ...');
});

it('truncates one serialized component field to its character cap', () => {
const data = createDetailedData(createComponentNode('x'.repeat(70_000), {}));

Expand Down
1 change: 1 addition & 0 deletions valdi/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -598,6 +598,7 @@ kt_jvm_test(
"@android_mvn//:org_junit_vintage_junit_vintage_engine",
],
deps = [
":valdi_android_support",
":valdi_android_test_support",
":valdi_java",
"//src/valdi_modules/src/valdi/valdi_test:valdi_test_kt",
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
package com.snap.valdi.support

import android.content.Intent
import android.content.pm.ApplicationInfo
import android.os.Bundle
import android.util.Log
import android.view.View
import androidx.appcompat.app.AppCompatActivity

Expand All @@ -12,6 +15,12 @@ import com.snap.valdi.ValdiRuntimeManager
import com.snap.valdi.ValdiRuntime
import com.snap.valdi.utils.Disposable
import com.snap.valdi.support.DefaultNavigator
import com.snapchat.client.valdi.NativeBridge

/** Intent extra used to select the Valdi debugger port for a debuggable application. */
const val VALDI_DEBUGGER_PORT_INTENT_EXTRA = "com.snap.valdi.DEBUGGER_PORT"

private const val VALDI_LOG_TAG = "Valdi"

/**
This class implements an Android activity where the root view
Expand Down Expand Up @@ -53,8 +62,29 @@ abstract class AppBootstrapActivity: AppCompatActivity() {
override fun onCreate(savedInstanceState: Bundle?) {
super.onCreate(savedInstanceState)

val debuggerPort = requestedValdiDebuggerPort(
intent,
applicationInfo.flags and ApplicationInfo.FLAG_DEBUGGABLE != 0,
)
loadNativeLibrary()
if (debuggerPort != null) {
// This process-wide override intentionally persists for this debug host process. When the intent
// omits the extra, this block is skipped so externally configured environment state remains unchanged.
setDebuggerPortEnvironment(debuggerPort)
}

bootstrapValdiRuntime()
}

protected open fun loadNativeLibrary() {
System.loadLibrary(getNativeLibName())
}

protected open fun setDebuggerPortEnvironment(debuggerPort: Int) {
NativeBridge.setDebuggerPortEnvironment(debuggerPort)
}

protected open fun bootstrapValdiRuntime() {
createRuntimeManager()
this.rootView = createAppRootView()
setContentView(this.rootView)
Expand Down Expand Up @@ -106,3 +136,21 @@ abstract class AppBootstrapActivity: AppCompatActivity() {
}
}
}

internal fun requestedValdiDebuggerPort(intent: Intent?, debuggable: Boolean): Int? {
if (!debuggable || intent == null || !intent.hasExtra(VALDI_DEBUGGER_PORT_INTENT_EXTRA)) {
return null
}

val port = intent.extras?.get(VALDI_DEBUGGER_PORT_INTENT_EXTRA) as? Int
if (port == null || port !in 1..65535) {
Log.w(
VALDI_LOG_TAG,
"Ignoring invalid Valdi debugger port from intent extra " +
"$VALDI_DEBUGGER_PORT_INTENT_EXTRA: <redacted> (expected an integer in 1...65535)",
)
return null
}

return port
}
1 change: 1 addition & 0 deletions valdi/src/java/com/snapchat/client/valdi/NativeBridge.java
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

public class NativeBridge {
public static native int getBuildOptions();
public static native void setDebuggerPortEnvironment(int debuggerPort);
public static native long createRuntimeManager(Object mainThreadDispatcher,
Object snapDrawingFrameScheduler,
Object viewManager,
Expand Down
9 changes: 9 additions & 0 deletions valdi/src/valdi/android/NativeBridge.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@
#endif

#include <android/native_window_jni.h>
#include <cstdlib>
#include <string>

inline ValdiAndroid::RuntimeWrapper* getRuntimeWrapper(jlong handle) {
return reinterpret_cast<ValdiAndroid::RuntimeWrapper*>(handle);
Expand Down Expand Up @@ -148,6 +150,12 @@ jint ValdiAndroid::NativeBridge::getBuildOptions(fbjni::alias_ref<fbjni::JClass>
return static_cast<jint>(buildOptions);
}

void ValdiAndroid::NativeBridge::setDebuggerPortEnvironment(fbjni::alias_ref<fbjni::JClass> /* clazz */,
jint debuggerPort) {
const auto debuggerPortString = std::to_string(debuggerPort);
setenv("VALDI_DEBUGGER_PORT", debuggerPortString.c_str(), 1);
}

jlong ValdiAndroid::NativeBridge::createRuntimeManager( // NOLINT
fbjni::alias_ref<fbjni::JClass> /* clazz */, // NOLINT
jobject mainThreadDispatcher,
Expand Down Expand Up @@ -2530,6 +2538,7 @@ jlong ValdiAndroid::NativeBridge::snapDrawingGetMaxRenderTargetSize(fbjni::alias
void ValdiAndroid::NativeBridge::registerNatives() {
javaClassStatic()->registerNatives({
makeNativeMethod("getBuildOptions", ValdiAndroid::NativeBridge::getBuildOptions),
makeNativeMethod("setDebuggerPortEnvironment", ValdiAndroid::NativeBridge::setDebuggerPortEnvironment),
makeNativeMethod("getAllRuntimeAttachedObjects", ValdiAndroid::NativeBridge::getAllRuntimeAttachedObjects),
makeNativeMethod("prepareRenderBackend", ValdiAndroid::NativeBridge::prepareRenderBackend),
makeNativeMethod("emitRuntimeManagerInitMetrics", ValdiAndroid::NativeBridge::emitRuntimeManagerInitMetrics),
Expand Down
1 change: 1 addition & 0 deletions valdi/src/valdi/android/NativeBridge.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ class NativeBridge : public fbjni::JavaClass<NativeBridge> {
static constexpr auto kJavaDescriptor = "Lcom/snapchat/client/valdi/NativeBridge;";

static jint getBuildOptions(fbjni::alias_ref<fbjni::JClass> clazz);
static void setDebuggerPortEnvironment(fbjni::alias_ref<fbjni::JClass> clazz, jint debuggerPort);

static jobject getAllRuntimeAttachedObjects(fbjni::alias_ref<fbjni::JClass> clazz, jlong runtimeManagerHandle);

Expand Down
3 changes: 2 additions & 1 deletion valdi/src/valdi/android/RuntimeManagerWrapper.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,8 @@ RuntimeManagerWrapper::RuntimeManagerWrapper(JavaEnv env,
_logger,
/* enableDebuggerService */ true,
/* disableHotReloader */ false,
/* isStandalone */ false);
/* isStandalone */ false,
std::nullopt);
_runtimeManager->postInit();
_runtimeManager->setKeepDebuggerServiceOnPause(static_cast<bool>(keepDebuggerServiceOnPause));
_runtimeManager->setApplicationId(_applicationId);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,12 @@ @implementation SCValdiBootstrappingAppDelegate {

- (BOOL)application:(UIApplication*)application didFinishLaunchingWithOptions:(NSDictionary*)launchOptions
{
_runtimeManager = [[SCValdiRuntimeManager alloc] init];
_runtimeManager = [SCValdiRuntimeManager new];

[_runtimeManager updateConfiguration:^(SCValdiConfiguration* configuration) {
configuration.allowDarkMode = YES;
// Bootstrap apps are local-development hosts, so keep their previous hot-reload behavior explicit.
configuration.enableDebuggerService = YES;
}];

id<SCValdiRuntimeProtocol> runtime = _runtimeManager.mainRuntime;
Expand Down
22 changes: 19 additions & 3 deletions valdi/src/valdi/ios/SCValdiRuntimeManager.mm
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,20 @@
#endif
}

static std::optional<uint32_t> SCValdiResolveDebuggerServicePort(SCValdiConfiguration* configuration) {
NSInteger debuggerServicePort = configuration.debuggerServicePort;
if (debuggerServicePort == 0) {
return std::nullopt;
}

if (debuggerServicePort < 0 || debuggerServicePort > 65535) {
SCLogValdiWarning(@"Ignoring invalid Valdi debugger service port: <redacted> (expected 1...65535)");
return std::nullopt;
}

return static_cast<uint32_t>(debuggerServicePort);
}

static void updateRuntimeManagersArray(void (^callback)(NSMutableArray<NSValue *> *runtimeManagers)) {
static dispatch_once_t onceToken;
static NSMutableArray<NSValue *> *kAllRuntimeManagers;
Expand Down Expand Up @@ -204,6 +218,7 @@ - (void)_initializeIfNeeded
_diskCache = Valdi::makeShared<Valdi::DiskCacheImpl>(resolveDocumentsDirectory());

id<SCNValdiKeychain> keychainStore = SCValdiCreateKeychainStore();
SCValdiConfiguration *configuration = [self _getOrCreateConfiguration];

_cppInstance = Valdi::makeShared<Valdi::RuntimeManager>(mainThreadDispatcher,
[self _javaScriptBridge],
Expand All @@ -213,9 +228,10 @@ - (void)_initializeIfNeeded
Valdi::PlatformTypeIOS,
Valdi::ThreadQoSClassMax,
logger,
/* enableDebuggerService */ true,
/* disableHotReloader */ false,
/* isStandalone */ false);
configuration.enableDebuggerService,
configuration.disableHotReloader,
/* isStandalone */ false,
SCValdiResolveDebuggerServicePort(configuration));
_cppInstance->postInit();
NSString *bundleIdentifier = [NSBundle mainBundle].bundleIdentifier;
_cppInstance->setApplicationId(ValdiIOS::StringFromNSString(bundleIdentifier));
Expand Down
3 changes: 2 additions & 1 deletion valdi/src/valdi/macos/SCValdiRuntime.mm
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,8 @@ - (instancetype)initWithUsingTemporaryCachesDirectory:(BOOL)usingTemporaryCaches
Valdi::strongSmallRef(&Valdi::ConsoleLogger::getLogger()),
/* enableDebuggerService */ true,
/* disableHotReloader */ false,
/* isStandalone */ true);
/* isStandalone */ true,
std::nullopt);
_runtimeManager->postInit();
_runtimeManager->applicationDidResume();
_runtimeManager->registerBytesAssetLoader(cachesImageCache);
Expand Down
48 changes: 48 additions & 0 deletions valdi/src/valdi/runtime/Debugger/DebuggerService.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,11 @@

#include "valdi_core/cpp/Utils/ContainerUtils.hpp"

#include <charconv>
#include <cstdlib>
#include <cstring>
#include <system_error>

namespace Valdi {

class DaemonClientTCPDataSender : public DaemonClientDataSender {
Expand Down Expand Up @@ -138,6 +143,10 @@ void DebuggerService::stop() {
});
}

uint32_t DebuggerService::getConfiguredPort() const {
return _debuggerPort;
}

uint16_t DebuggerService::getBoundPort() {
uint16_t boundPort = 0;
_dispatchQueue->sync([&]() {
Expand Down Expand Up @@ -316,4 +325,43 @@ uint32_t DebuggerService::resolveDebuggerPort(bool isStandalone) {
return isStandalone ? kStandaloneDebuggerPort : kMobileDebuggerPort;
}

DebuggerPortResolution DebuggerService::resolveDebuggerPortWithDiagnostics(
bool isStandalone,
std::optional<uint32_t> requestedPort) {
DebuggerPortResolution resolution{
resolveDebuggerPort(isStandalone),
std::nullopt,
std::nullopt,
};

if (requestedPort.has_value()) {
if (requestedPort.value() > 0 && requestedPort.value() <= 65535) {
resolution.port = requestedPort.value();
return resolution;
}
resolution.rejectedRequestedPort = requestedPort;
}

const char* overridePort = std::getenv("VALDI_DEBUGGER_PORT");

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🔴 High (cross-PR): This reads VALDI_DEBUGGER_PORT to set the on-device debugger TCP service port (default 13591/13592), but the CLI debugger (PR #154, server.ts) reads the same env var to set its browser UI HTTP port (default 8765) — the variable is overloaded to mean two different ports. Worse, #154's target discovery hardcodes 13591/13592 and never reads this var, so relocating the service port here is not picked up by the debugger UI (only a manual ?port= works). Failure scenario: export VALDI_DEBUGGER_PORT=14000 to move the service → the CLI UI also mis-binds to 14000 and still probes 13591/13592. Suggest distinct names (VALDI_DEBUGGER_SERVICE_PORT vs VALDI_DEBUGGER_UI_PORT) and having #154 discovery honor the configured service port.

if (overridePort != nullptr) {
uint32_t parsedPort = 0;
const char* overridePortEnd = overridePort + std::strlen(overridePort);
const auto parseResult = std::from_chars(overridePort, overridePortEnd, parsedPort);
if (parseResult.ec == std::errc() && parseResult.ptr == overridePortEnd && parsedPort > 0 &&
parsedPort <= 65535) {
resolution.port = parsedPort;
return resolution;
}

if (parseResult.ec == std::errc::result_out_of_range ||
(parseResult.ec == std::errc() && parseResult.ptr == overridePortEnd)) {
resolution.environmentError = DebuggerPortEnvironmentError::OutOfRange;
} else {
resolution.environmentError = DebuggerPortEnvironmentError::Malformed;
}
}

return resolution;
}

} // namespace Valdi
Loading
Loading